Re: [PATCH v3 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args

[Daniel Kiper]

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 <development@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