On Wed, 17 Nov 2021 20:10:21 +0100
Daniel Kiper <dki...@net-space.pl> wrote:
> On Tue, Oct 12, 2021 at 06:26:27PM -0500, Glenn Washburn wrote:
> > The crypto device modules should only be setting up the crypto devices and
> > not getting user input. This has the added benefit of simplifying the code
> > such that three essentially duplicate pieces of code are merged into one.
>
> Mostly nitpicks below...
>
> > Signed-off-by: Glenn Washburn <developm...@efficientek.com>
> > ---
> > grub-core/disk/cryptodisk.c | 52 ++++++++++++++++++++++++++++++-------
> > grub-core/disk/geli.c | 26 ++++---------------
> > grub-core/disk/luks.c | 27 +++----------------
> > grub-core/disk/luks2.c | 26 ++++---------------
> > include/grub/cryptodisk.h | 1 +
> > 5 files changed, 57 insertions(+), 75 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 577942088..a5f7b860c 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -1001,9 +1001,11 @@ grub_cryptodisk_scan_device_real (const char *name,
> > grub_disk_t source,
> > grub_cryptomount_args_t cargs)
> > {
> > - grub_err_t err;
> > + grub_err_t ret = GRUB_ERR_NONE;
> > grub_cryptodisk_t dev;
> > grub_cryptodisk_dev_t cr;
> > + int askpass = 0;
> > + char *part = NULL;
> >
> > dev = grub_cryptodisk_get_by_source_disk (source);
> >
> > @@ -1017,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
> > return grub_errno;
> > if (!dev)
> > continue;
> > -
> > - err = cr->recover_key (source, dev, cargs);
> > - if (err)
> > - {
> > - cryptodisk_close (dev);
> > - return err;
> > - }
> > +
> > + if (cargs->key_len == 0)
>
> I am OK with "if (!cargs->key_len)" for all kinds of numeric values...
>
> > + {
> > + /* Get the passphrase from the user, if no key data. */
> > + askpass = 1;
> > + if (source->partition)
>
> ...but not for the pointers and enums.
>
> s/if (source->partition)/if (source->partition != NULL)/
>
> > + part = grub_partition_get_name (source->partition);
> > + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> > + source->partition ? "," : "", part ? : "",
>
> s/source->partition ? "," : ""/source->partition != NULL ? "," : ""/
>
> s/part ? : ""/part != NULL ? part : "NO NAME"/?
For completeness, I missed a part of your suggestion in my original
response. I don't believe we should use "NO NAME" because a part ==
NULL means that the source device is not a partition (or that
grub_partition_get_name fails, which only happens if grub_malloc
fails). So the empty string is more appropriate. I could add an extra
tertiary operator to select "NO NAME" (or better "UNKNOWN"?) if
source->partition != NULL. I think this should be done in a different
patch though.
Glenn
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
