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

[Daniel Kiper]

On Tue, Jul 06, 2021 at 11:02:15AM +0200, Javier Martinez Canillas wrote:
> 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.
>
> 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;

static grub_efi_guid_t global = GRUB_EFI_GLOBAL_VARIABLE_GUID;

AFAICT this should generate smaller code. Hmmm... I think we should add
a preparatory patch which changes all GUID assignments like that one to
"static". Could you do that?

> +  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;
> +

grub_free(os_indications_supported) call is missing in this function.
So, we need an additional patch which fixes this up front.

> +  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"));

Most error messages start with lower case. Please fix this.

Daniel