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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1855 bytes --]

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.

[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
>  {
> +  grub_uint32_t check_boot : 1;
> +  grub_uint32_t found_uuid : 1;
> +  char *search_uuid;
>    grub_uint8_t *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.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]