[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: |
Patrick Steinhardt |
Subject: |
Re: [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct |
Date: |
Sun, 3 Oct 2021 21:16:09 +0200 |
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
signature.asc
Description: PGP signature
- Re: [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct,
Patrick Steinhardt <=