On Tue, Oct 12, 2021 at 06:26:29PM -0500, Glenn Washburn wrote:
> The member found_uuid was never used by the crypto-backends, but was used to
Ha! Could you make this patch second in this patch series? Then we could
avoid carrying over have_it/found_uuid cruft over succeeding patches.
> 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
> iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device
s/iff/if/
> will only return 1 if a search_uuid has been specified and a cryptodisk was
> successfully setup by a crypto-backend. So the return value of
> grub_cryptodisk_scan_device is almost equivalent to found_uuid, with the
> exception of the case where a mount is requested or an already opened
> crypto device.
>
> With this change grub_device_iterate will return 1 when
I like if you add "()" to function names in comments, etc.
> grub_cryptodisk_scan_device when a crypto device is successfully decrypted
I think one "when" is redundant here. Or something else is wrong.
> 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 <developm...@efficientek.com>
> ---
> grub-core/disk/cryptodisk.c | 9 ++++-----
> include/grub/cryptodisk.h | 1 -
> 2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 5b38606ed..8e5277314 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name,
>
> grub_cryptodisk_insert (dev, name, source);
>
> - cargs->found_uuid = 1;
> -
> goto cleanup;
> }
> goto cleanup;
> @@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name,
>
> if (err)
> grub_print_error ();
> - return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> + return (!err && cargs->search_uuid) ? 1 : 0;
err == GRUB_ERR_NONE && cargs->search_uuid != NULL
> }
>
> static grub_err_t
> @@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
>
> if (state[0].set) /* uuid */
> {
> + int found_uuid = 0;
Zero initialization seems redundant here.
> grub_cryptodisk_t dev;
>
> dev = grub_cryptodisk_get_by_uuid (args[0]);
> @@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
>
> cargs.check_boot = state[2].set;
> cargs.search_uuid = args[0];
> - grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> + found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device,
> &cargs);
>
> - if (!cargs.found_uuid)
> + if (!found_uuid)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
> return GRUB_ERR_NONE;
> }
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
