On Mon, 7 Feb 2022 12:12:14 +0100
Renaud Métrich <rmetr...@redhat.com> wrote:
> Please find inline the new patch integrating Glenn's comments (new
> "flags" option instead of "no-floppy" / "efidisk-only").
Thanks for making this inline, but its still not in a great format for
maintainers. Please use "git format-patch" and "git send-email" for the
next iteration. Its pretty easy to setup.
Also, please use the -v option to format-patch to increment the version
of the patch we're on (the next should be v3). Also create a new thread
for each new patch series sent to this list.
I suspect tht Daniel will also request that it be documented in the
commit message that no-floppy handling was changed. Perhaps a line like
"Refactor handling of --no-floppy option as well." Daniel might have
other ideas though.
> When using 'search' on EFI systems, we sometimes want to exclude devices
> that are not EFI disks (e.g. md, lvm).
> This is typically used when wanting to chainload when having a software
> raid (md) for EFI partition:
> with no option, 'search --file /EFI/redhat/shimx64.efi' sets root envvar
> to 'md/boot_efi' which cannot be used for chainloading since there is no
> effective EFI device behind.
>
> Signed-off-by: Renaud Métrich <rmetr...@redhat.com>
>
> diff --git a/grub-core/commands/search.c b/grub-core/commands/search.c
> index ed090b3af..57d26ced8 100644
> --- a/grub-core/commands/search.c
> +++ b/grub-core/commands/search.c
> @@ -47,7 +47,7 @@ struct search_ctx
> {
> const char *key;
> const char *var;
> - int no_floppy;
> + enum search_flags flags;
> char **hints;
> unsigned nhints;
> int count;
> @@ -62,9 +62,28 @@ iterate_device (const char *name, void *data)
> int found = 0;
>
> /* Skip floppy drives when requested. */
> - if (ctx->no_floppy &&
> + if (ctx->flags & SEARCH_FLAGS_NO_FLOPPY &&
> name[0] == 'f' && name[1] == 'd' && name[2] >= '0' && name[2] <=
> '9')
> - return 1;
> + return 0;
Ok, so this fixes a bug. At a minimum, that needs to be documented in
the commit message. I think there should be a separate patch with this
bug fix, since its unrelated. Daniel may accept it all as one patch.
I'll let him chime in.
> +
> + /* Limit to EFI disks when requested. */
> + if (ctx->flags & SEARCH_FLAGS_EFIDISK_ONLY)
> + {
> + grub_device_t dev;
> + dev = grub_device_open (name);
> + if (! dev)
> + {
> + grub_errno = GRUB_ERR_NONE;
> + return 0;
> + }
This indent formatting looks off. I think some tabs are getting
converted to 4 spaces.
> + if (! dev->disk || dev->disk->dev->id != GRUB_DISK_DEVICE_EFIDISK_ID)
> + {
> + grub_device_close (dev);
> + grub_errno = GRUB_ERR_NONE;
> + return 0;
> + }
Ditto.
> + grub_device_close (dev);
> + }
>
> #ifdef DO_SEARCH_FS_UUID
> #define compare_fn grub_strcasecmp
> @@ -261,13 +280,13 @@ try (struct search_ctx *ctx)
> }
>
> void
> -FUNC_NAME (const char *key, const char *var, int no_floppy,
> +FUNC_NAME (const char *key, const char *var, enum search_flags flags,
> char **hints, unsigned nhints)
> {
> struct search_ctx ctx = {
> .key = key,
> .var = var,
> - .no_floppy = no_floppy,
> + .flags = flags,
> .hints = hints,
> .nhints = nhints,
> .count = 0,
> diff --git a/grub-core/commands/search_wrap.c
> b/grub-core/commands/search_wrap.c
> index 47fc8eb99..464e6ebb1 100644
> --- a/grub-core/commands/search_wrap.c
> +++ b/grub-core/commands/search_wrap.c
> @@ -40,6 +40,7 @@ static const struct grub_arg_option options[] =
> N_("Set a variable to the first device found."), N_("VARNAME"),
> ARG_TYPE_STRING},
> {"no-floppy", 'n', 0, N_("Do not probe any floppy drive."), 0, 0},
> + {"efidisk-only", 0, 0, N_("Only probe EFI disks."), 0, 0},
> {"hint", 'h', GRUB_ARG_OPTION_REPEATABLE,
> N_("First try the device HINT. If HINT ends in comma, "
> "also try subpartitions"), N_("HINT"), ARG_TYPE_STRING},
> @@ -73,6 +74,7 @@ enum options
> SEARCH_FS_UUID,
> SEARCH_SET,
> SEARCH_NO_FLOPPY,
> + SEARCH_EFIDISK_ONLY,
> SEARCH_HINT,
> SEARCH_HINT_IEEE1275,
> SEARCH_HINT_BIOS,
> @@ -89,6 +91,7 @@ grub_cmd_search (grub_extcmd_context_t ctxt, int argc,
> char **args)
> const char *id = 0;
> int i = 0, j = 0, nhints = 0;
> char **hints = NULL;
> + enum search_flags flags;
>
> if (state[SEARCH_HINT].set)
> for (i = 0; state[SEARCH_HINT].args[i]; i++)
> @@ -180,15 +183,18 @@ grub_cmd_search (grub_extcmd_context_t ctxt, int
> argc, char **args)
> goto out;
> }
>
> + if (state[SEARCH_NO_FLOPPY].set)
> + flags |= SEARCH_FLAGS_NO_FLOPPY;
> +
> + if (state[SEARCH_EFIDISK_ONLY].set)
> + flags |= SEARCH_FLAGS_EFIDISK_ONLY;
> +
> if (state[SEARCH_LABEL].set)
> - grub_search_label (id, var, state[SEARCH_NO_FLOPPY].set,
> - hints, nhints);
> + grub_search_label (id, var, flags, hints, nhints);
> else if (state[SEARCH_FS_UUID].set)
> - grub_search_fs_uuid (id, var, state[SEARCH_NO_FLOPPY].set,
> - hints, nhints);
> + grub_search_fs_uuid (id, var, flags, hints, nhints);
> else if (state[SEARCH_FILE].set)
> - grub_search_fs_file (id, var, state[SEARCH_NO_FLOPPY].set,
> - hints, nhints);
> + grub_search_fs_file (id, var, flags, hints, nhints);
> else
> grub_error (GRUB_ERR_INVALID_COMMAND, "unspecified search type");
>
> diff --git a/include/grub/search.h b/include/grub/search.h
> index d80347df3..4190aeb2c 100644
> --- a/include/grub/search.h
> +++ b/include/grub/search.h
> @@ -19,11 +19,20 @@
> #ifndef GRUB_SEARCH_HEADER
> #define GRUB_SEARCH_HEADER 1
>
> -void grub_search_fs_file (const char *key, const char *var, int no_floppy,
> +enum search_flags
> + {
> + SEARCH_FLAGS_NO_FLOPPY = 1,
> + SEARCH_FLAGS_EFIDISK_ONLY = 2
> + };
> +
> +void grub_search_fs_file (const char *key, const char *var,
> + enum search_flags flags,
> char **hints, unsigned nhints);
> -void grub_search_fs_uuid (const char *key, const char *var, int no_floppy,
> +void grub_search_fs_uuid (const char *key, const char *var,
> + enum search_flags flags,
> char **hints, unsigned nhints);
> -void grub_search_label (const char *key, const char *var, int no_floppy,
> +void grub_search_label (const char *key, const char *var,
> + enum search_flags flags,
> char **hints, unsigned nhints);
>
> #endif
This is weird here. It looks like the #endif has a space before it
on the line (but it doesn't in master). Perhaps an artifact of how you
did the inline? Using git to format and send the patch should fix these
issues.
Glenn
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
