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

[Glenn Washburn]

On Mon, 4 Oct 2021 10:43:50 +0200
Patrick Steinhardt <ps@pks.im> wrote:

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

Make sense. I'll add that in. I'm seeing now that the comment as is
could lead one to believe that its the master key data, which its not.

Glenn