[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomou
From: |
Glenn Washburn |
Subject: |
Re: [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct |
Date: |
Sun, 3 Oct 2021 18:57:45 -0500 |
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