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

[Glenn Washburn <development@efficientek.com>]

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). 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.

> 
> [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 */

> >    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? I
didn't add one for found_uuid because the next patch gets rid of it
anyway.

Glenn