Re: [PATCH v3 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args

[Glenn Washburn]

On Thu, 18 Nov 2021 15:25:44 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, Oct 12, 2021 at 06:26:29PM -0500, Glenn Washburn wrote:
> > The member found_uuid was never used by the crypto-backends, but was used to
> 
> Ha! Could you make this patch second in this patch series? Then we could
> avoid carrying over have_it/found_uuid cruft over succeeding patches.

Sure, I was avoiding do that work, but since you've requested it, I'll
take a stab at it. I'm thinking I'll make it the first patch though, so
its independent from the rest of the series.

> > determine if a crypto-backend successfully mounted a cryptodisk with a given
> > uuid. This is not needed however, because grub_device_iterate will return 1
> > iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device
> 
> s/iff/if/

"iff" is short hand for "if and only if". I'll expand it.

> > will only return 1 if a search_uuid has been specified and a cryptodisk was
> > successfully setup by a crypto-backend. So the return value of
> > grub_cryptodisk_scan_device is almost equivalent to found_uuid, with the
> > exception of the case where a mount is requested or an already opened
> > crypto device.
> >
> > With this change grub_device_iterate will return 1 when
> 
> I like if you add "()" to function names in comments, etc.
> 
> > grub_cryptodisk_scan_device when a crypto device is successfully decrypted
> 
> I think one "when" is redundant here. Or something else is wrong.

Indeed, I'll fix this.

> > or when the source device has already been successfully opened. Prior to
> > this change, trying to mount an already successfully opened device would
> > trigger an error with the message "no such cryptodisk found", which is at
> > best misleading. The mount should silently succeed in this case, which is
> > what happens with this patch.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 9 ++++-----
> >  include/grub/cryptodisk.h   | 1 -
> >  2 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 5b38606ed..8e5277314 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name,
> >
> >      grub_cryptodisk_insert (dev, name, source);
> >
> > -    cargs->found_uuid = 1;
> > -
> >      goto cleanup;
> >    }
> >    goto cleanup;
> > @@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name,
> >
> >    if (err)
> >      grub_print_error ();
> > -  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> > +  return (!err && cargs->search_uuid) ? 1 : 0;
> 
> err == GRUB_ERR_NONE && cargs->search_uuid != NULL
> 
> >  }
> >
> >  static grub_err_t
> > @@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> > argc, char **args)
> >
> >    if (state[0].set) /* uuid */
> >      {
> > +      int found_uuid = 0;
> 
> Zero initialization seems redundant here.

It is. I'll remove the initializer.

> >        grub_cryptodisk_t dev;
> >
> >        dev = grub_cryptodisk_get_by_uuid (args[0]);
> > @@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> > argc, char **args)
> >
> >        cargs.check_boot = state[2].set;
> >        cargs.search_uuid = args[0];
> > -      grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > +      found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, 
> > &cargs);
> >
> > -      if (!cargs.found_uuid)
> > +      if (!found_uuid)
> >     return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
> >        return GRUB_ERR_NONE;
> >      }

Glenn