[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 2/7] cryptodisk: Refactor to discard have_it global
From: |
Daniel Kiper |
Subject: |
Re: [PATCH v4 2/7] cryptodisk: Refactor to discard have_it global |
Date: |
Wed, 8 Dec 2021 17:37:19 +0100 |
User-agent: |
NeoMutt/20170113 (1.7.2) |
On Sat, Dec 04, 2021 at 01:15:45AM -0600, Glenn Washburn wrote:
> The global "have_it" was never used by the crypto-backends, but was used to
> determine if a crypto-backend successfully mounted a cryptodisk with a given
> uuid. This is not needed however, because grub_device_iterate() will return
> 1 if and only if grub_cryptodisk_scan_device() returns 1. And
> grub_cryptodisk_scan_device() will now only return 1 if a search_uuid has
> been specified and a cryptodisk was successfully setup by a crypto-backend.
>
> With this change grub_device_iterate() will return 1 when a crypto device is
> successfully decrypted or when the source device has already been
> successfully opened. Prior to this change, trying to mount an already
> successfully opened device would trigger an error with the message "no such
> cryptodisk found", which is at best misleading. The mount should silently
> succeed in this case, which is what happens with this patch.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
> grub-core/disk/cryptodisk.c | 29 ++++++++++++++++++-----------
> include/grub/err.h | 1 +
> 2 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 90f82b2d3..9224105ac 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -983,7 +983,7 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>
> #endif
>
> -static int check_boot, have_it;
> +static int check_boot;
> static char *search_uuid;
>
> static void
> @@ -1005,7 +1005,12 @@ grub_cryptodisk_scan_device_real (const char *name,
> grub_disk_t source)
> dev = grub_cryptodisk_get_by_source_disk (source);
>
> if (dev)
> - return GRUB_ERR_NONE;
> + {
> + if (grub_strcasecmp (search_uuid, dev->uuid) == 0)
> + return GRUB_ERR_NONE;
> + else
> + return GRUB_ERR_EXISTS;
Hmmm... This looks strange. Could you explain why you return
GRUB_ERR_EXISTS if UUIDs do not match?
> + }
>
> FOR_CRYPTODISK_DEVS (cr)
> {
> @@ -1024,11 +1029,9 @@ grub_cryptodisk_scan_device_real (const char *name,
> grub_disk_t source)
>
> grub_cryptodisk_insert (dev, name, source);
>
> - have_it = 1;
> -
> return GRUB_ERR_NONE;
> }
> - return GRUB_ERR_NONE;
> + return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle
> this device");
Could you enlighten me why GRUB_ERR_NONE changes to GRUB_ERR_BAD_MODULE?
> }
>
> #ifdef GRUB_UTIL
> @@ -1096,10 +1099,14 @@ grub_cryptodisk_scan_device (const char *name,
> err = grub_cryptodisk_scan_device_real (name, source);
>
> grub_disk_close (source);
> -
> - if (err)
> +
> + /*
> + * Do not print error when err is GRUB_ERR_BAD_MODULE to avoid many
> unhelpful
> + * error messages.
> + */
> + if (err != GRUB_ERR_NONE && err != GRUB_ERR_EXISTS && err !=
> GRUB_ERR_BAD_MODULE)
> grub_print_error ();
> - return have_it && search_uuid ? 1 : 0;
> + return (err == GRUB_ERR_NONE && search_uuid != NULL);
> }
>
> static grub_err_t
> @@ -1110,9 +1117,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
> if (argc < 1 && !state[1].set && !state[2].set)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
>
> - have_it = 0;
> if (state[0].set)
> {
> + int found_uuid;
> grub_cryptodisk_t dev;
>
> dev = grub_cryptodisk_get_by_uuid (args[0]);
> @@ -1125,10 +1132,10 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
>
> check_boot = state[2].set;
> search_uuid = args[0];
> - grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
> + found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
> search_uuid = NULL;
>
> - if (!have_it)
> + if (!found_uuid)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
> return GRUB_ERR_NONE;
> }
> diff --git a/include/grub/err.h b/include/grub/err.h
> index b08d5d0de..55219cdcc 100644
> --- a/include/grub/err.h
> +++ b/include/grub/err.h
> @@ -57,6 +57,7 @@ typedef enum
> GRUB_ERR_MENU,
> GRUB_ERR_TIMEOUT,
> GRUB_ERR_IO,
> + GRUB_ERR_EXISTS,
> GRUB_ERR_ACCESS_DENIED,
> GRUB_ERR_EXTRACTOR,
> GRUB_ERR_NET_BAD_ADDRESS,
Daniel
[PATCH v4 4/7] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules, Glenn Washburn, 2021/12/04
[PATCH v4 5/7] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk, Glenn Washburn, 2021/12/04
[PATCH v4 6/7] cryptodisk: Move global variables into grub_cryptomount_args struct, Glenn Washburn, 2021/12/04
[PATCH v4 7/7] cryptodisk: Improve handling of partition name in cryptomount password prompt, Glenn Washburn, 2021/12/04