* [PATCH] search: new --efidisk-only option on EFI systems
@ 2022-02-01 10:36 Renaud Métrich
2022-02-02 16:01 ` Robbie Harwood
2022-02-04 22:28 ` Glenn Washburn
0 siblings, 2 replies; 7+ messages in thread
From: Renaud Métrich @ 2022-02-01 10:36 UTC (permalink / raw)
To: grub-devel; +Cc: Robbie Harwood, Javier Martinez Canillas
[-- Attachment #1.1.1.1: Type: text/plain, Size: 1188 bytes --]
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:
~~~
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
[-- Attachment #1.1.1.2: Type: text/html, Size: 1689 bytes --]
[-- Attachment #1.1.2: 0001-search-new-efidisk-only-option-on-EFI-systems.patch --]
[-- Type: text/x-patch, Size: 5213 bytes --]
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
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;
+
+#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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] search: new --efidisk-only option on EFI systems
2022-02-01 10:36 [PATCH] search: new --efidisk-only option on EFI systems Renaud Métrich
@ 2022-02-02 16:01 ` Robbie Harwood
2022-02-04 22:28 ` Glenn Washburn
1 sibling, 0 replies; 7+ messages in thread
From: Robbie Harwood @ 2022-02-02 16:01 UTC (permalink / raw)
To: Renaud Métrich, grub-devel; +Cc: Javier Martinez Canillas
[-- Attachment #1: Type: text/plain, Size: 689 bytes --]
Renaud Métrich <rmetrich@redhat.com> writes:
> 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>
The patch fixes the systems in question, and I don't see a way for it to
break other things.
Reviewed-by: Robbie Harwood <rharwood@redhat.com>
Be well,
--Robbie
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] search: new --efidisk-only option on EFI systems
2022-02-01 10:36 [PATCH] search: new --efidisk-only option on EFI systems Renaud Métrich
2022-02-02 16:01 ` Robbie Harwood
@ 2022-02-04 22:28 ` Glenn Washburn
2022-02-07 9:27 ` Renaud Métrich
1 sibling, 1 reply; 7+ messages in thread
From: Glenn Washburn @ 2022-02-04 22:28 UTC (permalink / raw)
To: Renaud Métrich
Cc: The development of GNU GRUB, Robbie Harwood, Javier Martinez Canillas
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] search: new --efidisk-only option on EFI systems
2022-02-04 22:28 ` Glenn Washburn
@ 2022-02-07 9:27 ` Renaud Métrich
2022-02-07 11:12 ` Renaud Métrich
0 siblings, 1 reply; 7+ messages in thread
From: Renaud Métrich @ 2022-02-07 9:27 UTC (permalink / raw)
To: grub-devel
[-- Attachment #1.1: Type: text/plain, Size: 8634 bytes --]
See inline, sorry for the format.
Le 2/4/22 à 23:28, Glenn Washburn a écrit :
> 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.
OK, will change this to a new "flags" and make "no_floppy" be a flag.
>> 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
Actually I think this is a bug in the --no-floppy code: by returning 1,
the iterate stops, which is probably not what is expected at all.
I hence fixed this as part of my code but it's not related indeed.
>> +
>> +#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
>>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] search: new --efidisk-only option on EFI systems
2022-02-07 9:27 ` Renaud Métrich
@ 2022-02-07 11:12 ` Renaud Métrich
2022-02-07 21:32 ` Glenn Washburn
0 siblings, 1 reply; 7+ messages in thread
From: Renaud Métrich @ 2022-02-07 11:12 UTC (permalink / raw)
To: grub-devel
[-- Attachment #1.1: Type: text/plain, Size: 15432 bytes --]
Please find inline the new patch integrating Glenn's comments (new
"flags" option instead of "no-floppy" / "efidisk-only").
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..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;
+
+ /* 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;
+ }
+ 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);
+ }
#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
--
2.34.1
Le 2/7/22 à 10:27, Renaud Métrich a écrit :
> See inline, sorry for the format.
>
> Le 2/4/22 à 23:28, Glenn Washburn a écrit :
>> 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.
> OK, will change this to a new "flags" and make "no_floppy" be a flag.
>>> 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
>
> Actually I think this is a bug in the --no-floppy code: by returning
> 1, the iterate stops, which is probably not what is expected at all.
>
> I hence fixed this as part of my code but it's not related indeed.
>
>>> +
>>> +#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
>>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> Grub-devel@gnu.org
>> https://lists.gnu.org/mailman/listinfo/grub-devel
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] search: new --efidisk-only option on EFI systems
2022-02-07 11:12 ` Renaud Métrich
@ 2022-02-07 21:32 ` Glenn Washburn
2022-02-08 7:40 ` Renaud Métrich
0 siblings, 1 reply; 7+ messages in thread
From: Glenn Washburn @ 2022-02-07 21:32 UTC (permalink / raw)
To: Renaud Métrich; +Cc: The development of GNU GRUB
On Mon, 7 Feb 2022 12:12:14 +0100
Renaud Métrich <rmetrich@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 <rmetrich@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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] search: new --efidisk-only option on EFI systems
2022-02-07 21:32 ` Glenn Washburn
@ 2022-02-08 7:40 ` Renaud Métrich
0 siblings, 0 replies; 7+ messages in thread
From: Renaud Métrich @ 2022-02-08 7:40 UTC (permalink / raw)
To: development; +Cc: The development of GNU GRUB
[-- Attachment #1.1: Type: text/plain, Size: 7816 bytes --]
So Glenn, I'm new to Grub developer, in the past I was relying on Javier
Martinez Canillas ...
Just sent v3.
Le 2/7/22 à 22:32, Glenn Washburn a écrit :
> On Mon, 7 Feb 2022 12:12:14 +0100
> Renaud Métrich <rmetrich@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 <rmetrich@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
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-02-08 7:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-01 10:36 [PATCH] search: new --efidisk-only option on EFI systems Renaud Métrich
2022-02-02 16:01 ` Robbie Harwood
2022-02-04 22:28 ` Glenn Washburn
2022-02-07 9:27 ` Renaud Métrich
2022-02-07 11:12 ` Renaud Métrich
2022-02-07 21:32 ` Glenn Washburn
2022-02-08 7:40 ` Renaud Métrich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.