[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 4/4] cryptodisk: Remove unneeded found_uuid from cryptomou
From: |
Glenn Washburn |
Subject: |
Re: [PATCH v3 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args |
Date: |
Thu, 2 Dec 2021 00:51:09 -0600 |
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
- Re: [PATCH v3 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args,
Glenn Washburn <=