On Sun, Oct 03, 2021 at 06:57:45PM -0500, Glenn Washburn wrote: > On Sun, 3 Oct 2021 21:16:09 +0200 > Patrick Steinhardt <p...@pks.im> wrote: > > > On Mon, Sep 27, 2021 at 06:14:02PM -0500, Glenn Washburn wrote: > > > Signed-off-by: Glenn Washburn <developm...@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
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel