Re: [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct

[Patrick Steinhardt]

On Sun, Oct 03, 2021 at 06:57:45PM -0500, Glenn Washburn wrote:
> On Sun, 3 Oct 2021 21:16:09 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > On Mon, Sep 27, 2021 at 06:14:02PM -0500, Glenn Washburn wrote:
> > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > ---
> > >  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> > >  grub-core/disk/geli.c       |  9 ++++-----
> > >  grub-core/disk/luks.c       | 11 +++++------
> > >  grub-core/disk/luks2.c      |  6 +++---
> > >  include/grub/cryptodisk.h   |  6 ++++--
> > >  5 files changed, 25 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > > index 86eaabe60..5e153ee0a 100644
> > > --- a/grub-core/disk/cryptodisk.c
> > > +++ b/grub-core/disk/cryptodisk.c
> > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> > >  
> > >  #endif
> > >  
> > > -static int check_boot, have_it;
> > 
> > We should really just get rid of `have_it`. It's only used in three
> > locations, and we only require it because
> > `grub_cryptodisk_scan_device_real` doesn't properly tell us whether it
> > was found or not. Using a parameter struct to pass back this value to
> > the caller feels like a code smell, and only papers over this weird
> > usage.
> > 
> > Anyway, your patch doesn't make it any worse, so we may just as well fix
> > it after this series has landed.
> 
> I guess you wrote this before taknig a look at the next patch in this
> series, which does get rid of have_it (renamed to found_uuid).

Indeed.

> The
> intent of this patch was not to change the existing behavior, just get
> rid of the globals. I could've moved the following patch before this
> one, in which case have_it wouldn't exist here. But for historical
> reasons it was easier not too.

I don't particularly mind the order, so your current version is good
enough for me.

> > 
> > [snip]
> > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > > index 5bd970692..230167ab0 100644
> > > --- a/include/grub/cryptodisk.h
> > > +++ b/include/grub/cryptodisk.h
> > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t
> > >  
> > >  struct grub_cryptomount_args
> > >  {
> 
>   /* Flag to indicate that only bootable volumes should be decrypted */ 
> 
> > > +  grub_uint32_t check_boot : 1;
> > > +  grub_uint32_t found_uuid : 1;
> 
>   /* Only volumes matching this UUID should be decrpyted */
> 
> > > +  char *search_uuid;
> 
>   /* Key data used to decrypt voume */

This can either contain the user-provided password or contents of a key
file, right? Might be worthwhile to document that.

> > >    grub_uint8_t *key_data;
> 
>   /* Length of key_data */
> 
> > >    grub_size_t key_len;
> > >  };
> > 
> > Would be nice to have comments here which explain what these parameters
> > do. for `key_data` and `key_len` it's obvious enough, but as a reader I
> > wouldn't really know what `check_boot` ought to indicate.
> 
> I was trying to use the same names to provide continuity (except for
> have_it which is badly named). check_boot might better be named as
> only_boot. I can add some comments. I agree that as a reader I like to
> see descriptions of struct fields. How do the comments above look?

Thanks. Except for the one addendum above they look nice to me.

> I
> didn't add one for found_uuid because the next patch gets rid of it
> anyway.
> 
> Glenn

Fair enough.

Patrick

signature.asc
Description: PGP signature