Re: [PATCH] search: new --efidisk-only option on EFI systems

[Glenn Washburn]

On Tue, 1 Feb 2022 11:36:01 +0100
Renaud Métrich <rmetrich@redhat.com> wrote:

> 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.
> 
> Example of "grub.cfg" file used to chainload when system boots over the 
> network:

In the future, please submit patches inline and not attached. Otherwise
it makes it more difficult to respond to its contents. I've added the
patch inline in my response and quoted.

> 
> ~~~
> 
> menuentry 'Chainload Grub2 EFI from ESP' --id local {
> 
>    unset root
> 
>    search --file --no-floppy --efidisk-only --set=root /EFI/BOOT/BOOTX64.EFI
> 
>    if [ -f ($root)/EFI/BOOT/grubx64.efi ]; then
> 
>      chainloader ($root)/EFI/BOOT/grubx64.efi
> 
>    elif [ -f ($root)/EFI/redhat/shimx64.efi ]; then
> 
>      chainloader ($root)/EFI/redhat/shimx64.efi
> 
>    elif [ -f ($root)/EFI/redhat/grubx64.efi ]; then
> 
>      chainloader ($root)/EFI/redhat/grubx64.efi
> 
>    fi
> 
> }
> 
> ~~~
> 
> 
> This patch has been tested on QEMU/KVM systems and VMWare VMs (at 
> hardware level 6.7 and 7.0u2).
> 
> Related Red Hat BZ (public): 
> https://bugzilla.redhat.com/show_bug.cgi?id=2048904
> 


> From 46a8693953333e08fd2df4f722483b8db0f7da62 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Renaud=20M=C3=A9trich?= <rmetrich@redhat.com>
> Date: Tue, 1 Feb 2022 07:17:24 +0100
> Subject: [PATCH] search: new --efidisk-only option on EFI systems
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> 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 <rmetrich@redhat.com>
> 
> diff --git a/grub-core/commands/search.c b/grub-core/commands/search.c
> index ed090b3af..5e1e88643 100644
> --- a/grub-core/commands/search.c
> +++ b/grub-core/commands/search.c
> @@ -48,6 +48,9 @@ struct search_ctx
>    const char *key;
>    const char *var;
>    int no_floppy;
> +#ifdef GRUB_MACHINE_EFI
> +  int efidisk_only;
> +#endif

I think it would be cleaner to have a "flags" member here where right
now there would only be SEARCH_FLAGS_NO_FLOPPY and
SEARCH_FLAGS_EFIDISK_ONLY. This way if in the future someone wants to
add another filter to search they just add a flag instead of updating
all the function signatures. And for this patch it will remove the need
for most of the #ifdefs.

>    char **hints;
>    unsigned nhints;
>    int count;
> @@ -64,7 +67,28 @@ iterate_device (const char *name, void *data)
>    /* Skip floppy drives when requested.  */
>    if (ctx->no_floppy &&
>        name[0] == 'f' && name[1] == 'd' && name[2] >= '0' && name[2]
> <= '9')
> -    return 1;
> +    return 0;

So this function now returns success if --no-floppy was requested and
we've encountered a floppy? Am I missing something? If this is
desirable, perhaps it should be documented.

Glenn

> +
> +#ifdef GRUB_MACHINE_EFI
> +  /* Limit to EFI disks when requested.  */
> +  if (ctx->efidisk_only)
> +    {
> +      grub_device_t dev;
> +      dev = grub_device_open (name);
> +      if (! dev)
> +     {
> +       grub_errno = GRUB_ERR_NONE;
> +       return 0;
> +     }
> +      if (! dev->disk || dev->disk->dev->id !=
> GRUB_DISK_DEVICE_EFIDISK_ID)
> +     {
> +       grub_device_close (dev);
> +       grub_errno = GRUB_ERR_NONE;
> +       return 0;
> +     }
> +      grub_device_close (dev);
> +    }
> +#endif
>  
>  #ifdef DO_SEARCH_FS_UUID
>  #define compare_fn grub_strcasecmp
> @@ -262,12 +286,18 @@ try (struct search_ctx *ctx)
>  
>  void
>  FUNC_NAME (const char *key, const char *var, int no_floppy,
> +#ifdef GRUB_MACHINE_EFI
> +        int efidisk_only,
> +#endif
>          char **hints, unsigned nhints)
>  {
>    struct search_ctx ctx = {
>      .key = key,
>      .var = var,
>      .no_floppy = no_floppy,
> +#ifdef GRUB_MACHINE_EFI
> +    .efidisk_only = efidisk_only,
> +#endif
>      .hints = hints,
>      .nhints = nhints,
>      .count = 0,
> @@ -303,8 +333,12 @@ grub_cmd_do_search (grub_command_t cmd
> __attribute__ ((unused)), int argc, if (argc == 0)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("one argument
> expected")); 
> -  FUNC_NAME (args[0], argc == 1 ? 0 : args[1], 0, (args + 2),
> -          argc > 2 ? argc - 2 : 0);
> +  FUNC_NAME (args[0], argc == 1 ? 0 : args[1], 0,
> +#ifdef GRUB_MACHINE_EFI
> +          /* efidisk_only */
> +          0,
> +#endif
> +          (args + 2), argc > 2 ? argc - 2 : 0);
>  
>    return grub_errno;
>  }
> diff --git a/grub-core/commands/search_wrap.c
> b/grub-core/commands/search_wrap.c index 47fc8eb99..aed75b236 100644
> --- a/grub-core/commands/search_wrap.c
> +++ b/grub-core/commands/search_wrap.c
> @@ -40,6 +40,9 @@ 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}, +#ifdef GRUB_MACHINE_EFI
> +    {"efidisk-only", 0, 0, N_("Only probe EFI disks."), 0, 0},
> +#endif
>      {"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 +76,9 @@ enum options
>      SEARCH_FS_UUID,
>      SEARCH_SET,
>      SEARCH_NO_FLOPPY,
> +#ifdef GRUB_MACHINE_EFI
> +    SEARCH_EFIDISK_ONLY,
> +#endif
>      SEARCH_HINT,
>      SEARCH_HINT_IEEE1275,
>      SEARCH_HINT_BIOS,
> @@ -182,12 +188,21 @@ grub_cmd_search (grub_extcmd_context_t ctxt,
> int argc, char **args) 
>    if (state[SEARCH_LABEL].set)
>      grub_search_label (id, var, state[SEARCH_NO_FLOPPY].set, 
> +#ifdef GRUB_MACHINE_EFI
> +                    state[SEARCH_EFIDISK_ONLY].set,
> +#endif
>                      hints, nhints);
>    else if (state[SEARCH_FS_UUID].set)
>      grub_search_fs_uuid (id, var, state[SEARCH_NO_FLOPPY].set,
> +#ifdef GRUB_MACHINE_EFI
> +                      state[SEARCH_EFIDISK_ONLY].set,
> +#endif
>                        hints, nhints);
>    else if (state[SEARCH_FILE].set)
>      grub_search_fs_file (id, var, state[SEARCH_NO_FLOPPY].set, 
> +#ifdef GRUB_MACHINE_EFI
> +                      state[SEARCH_EFIDISK_ONLY].set,
> +#endif
>                        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..fc058add2 100644
> --- a/include/grub/search.h
> +++ b/include/grub/search.h
> @@ -20,10 +20,19 @@
>  #define GRUB_SEARCH_HEADER 1
>  
>  void grub_search_fs_file (const char *key, const char *var, int
> no_floppy, +#ifdef GRUB_MACHINE_EFI
> +                       int efidisk_only,
> +#endif
>                         char **hints, unsigned nhints);
>  void grub_search_fs_uuid (const char *key, const char *var, int
> no_floppy, +#ifdef GRUB_MACHINE_EFI
> +                       int efidisk_only,
> +#endif
>                         char **hints, unsigned nhints);
>  void grub_search_label (const char *key, const char *var, int
> no_floppy, +#ifdef GRUB_MACHINE_EFI
> +                     int efidisk_only,
> +#endif
>                       char **hints, unsigned nhints);
>  
>  #endif
> -- 
> 2.34.1
>