Re: [PATCH 2/2] efi: Print an error if boot to firmware setup is not supported

[Paul Menzel]

Dear Javier,


Thank you for this patch.


Am 06.07.21 um 11:02 schrieb Javier Martinez Canillas:
> The "fwsetup" command is only registered if the firmware supports booting
> to the firmware setup UI. But it could be possible that the GRUB config
> already contains a "fwsetup" entry, because it was generated in a machine
> that has support for this feature.
>
> To prevent users getting a "can't find command `fwsetup`" error if it is
> not supported by the firmware, let's just always register the command but
> print a more accurate message if the firmware doesn't support this option.

(I’d always paste the message in the commit message too.)

> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>   grub-core/commands/efi/efifwsetup.c | 43 +++++++++++++++--------------
>   1 file changed, 23 insertions(+), 20 deletions(-)
>
> diff --git a/grub-core/commands/efi/efifwsetup.c 
> b/grub-core/commands/efi/efifwsetup.c
> index eaca0328388..328c45e82e0 100644
> --- a/grub-core/commands/efi/efifwsetup.c
> +++ b/grub-core/commands/efi/efifwsetup.c
> @@ -27,6 +27,25 @@
>   
>   GRUB_MOD_LICENSE ("GPLv3+");
>   
> +static grub_efi_boolean_t
> +efifwsetup_is_supported (void)
> +{
> +  grub_efi_uint64_t *os_indications_supported = NULL;
> +  grub_size_t oi_size = 0;
> +  grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
> +
> +  grub_efi_get_variable ("OsIndicationsSupported", &global, &oi_size,
> +                        (void **) &os_indications_supported);
> +
> +  if (!os_indications_supported)
> +    return 0;
> +
> +  if (*os_indications_supported & GRUB_EFI_OS_INDICATIONS_BOOT_TO_FW_UI)
> +    return 1;
> +
> +  return 0;
> +}
> +
>   static grub_err_t
>   grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
>                   int argc __attribute__ ((unused)),
> @@ -38,6 +57,10 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ 
> ((unused)),
>     grub_size_t oi_size;
>     grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
>   
> +  if (!efifwsetup_is_supported ())
> +         return grub_error (GRUB_ERR_INVALID_COMMAND,
> +                            N_("Reboot to firmware setup is not supported"));
> +

I’d add: “… by the current firmware”, so it’s clear where to look.


Kind regards,

Paul


>     grub_efi_get_variable ("OsIndications", &global, &oi_size,
>                          (void **) &old_os_indications);
>   
> @@ -56,28 +79,8 @@ grub_cmd_fwsetup (grub_command_t cmd __attribute__ ((unused)),
>   
>   static grub_command_t cmd = NULL;
>   
> -static grub_efi_boolean_t
> -efifwsetup_is_supported (void)
> -{
> -  grub_efi_uint64_t *os_indications_supported = NULL;
> -  grub_size_t oi_size = 0;
> -  grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;
> -
> -  grub_efi_get_variable ("OsIndicationsSupported", &global, &oi_size,
> -                        (void **) &os_indications_supported);
> -
> -  if (!os_indications_supported)
> -    return 0;
> -
> -  if (*os_indications_supported & GRUB_EFI_OS_INDICATIONS_BOOT_TO_FW_UI)
> -    return 1;
> -
> -  return 0;
> -}
> -
>   GRUB_MOD_INIT (efifwsetup)
>   {
> -  if (efifwsetup_is_supported ())
>       cmd = grub_register_command ("fwsetup", grub_cmd_fwsetup, NULL,
>                                  N_("Reboot into firmware setup menu."));
>