Re: [PATCH RFC] Proposition of a --auto-nvram option for grub-install

[Daniel Kiper]

On Thu, Apr 12, 2018 at 07:09:58PM +0200, Lukasz Zemczak wrote:
> Hello everyone,
>
> I'm writing to this list since I would like to get some feedback on an
> additional option to the grub-install tool we would find very
> convenient to have. The diff is attached to the e-mail (also available
> as a pastebin [1]).
>
> The idea of the --auto-nvram (the name is just a proposition) flag is
> a bit similar to what --no-nvram does. After providing the option
> during grub-install, the tool will attempt to guess if there is access
> to NVRAM variables for EFI and/or IEEE1275 and, if yes, perform the
> usual variable updates. If no access to the NVRAM is available the
> whole thing is handled somewhat similar to --no-nvram + a warning

Make sense for me but I am not sure about "a warning". Hmmm...
Is it really needed?

> message displayed. Rationale:
>
> We would like to use this in Ubuntu for cases of dual BIOS/EFI
> bootloaders installed (at the same time), helpful for the situation of
> calling grub-install --target=x86_64-efi from the shim-efi package on
> a BIOS legacy-mode booted machine. For this legacy-mode case when
> running on a EFI-enabled device, currently this causes grub-install to
> fail as obviously there is no access to the NVRAM and no --no-nvram is
> given. We don't want to unconditionally append --no-nvram as this is
> not what we want to happen for the case of a system that is actually
> booted in EFI-mode. With this flag, we would be simply performing a
> grub-install --target=x86_64-efi --auto-nvram unconditionally which
> would do the right thing in both cases, allowing for a much simpler
> handling of this dual-bootloader case in Ubuntu. Having it being done
> inside grub-installer makes everything much cleaner.
>
> This is of course just a proposition about which I wanted to get some
> feedback from people that know the codebase the most. It's my first
> time working on the grub project so apologies for any flukes or
> silliness in the code or the idea itself.
>
> Thank you!
>
> Best regards,
>
> [1] http://paste.ubuntu.com/p/cWR3k3NZgF/

Next time please use git format-patch/send-email to send the patches.

> diff --git a/grub-core/osdep/basic/no_platform.c 
> b/grub-core/osdep/basic/no_platform.c
> index d76c34c14..b39e97f48 100644
> --- a/grub-core/osdep/basic/no_platform.c
> +++ b/grub-core/osdep/basic/no_platform.c
> @@ -25,7 +25,7 @@
>
>  void
>  grub_install_register_ieee1275 (int is_prep, const char *install_device,
> -                             int partno, const char *relpath)
> +                             int partno, const char *relpath, int 
> detect_nvram)
>  {
>    grub_util_error ("%s", _("no IEEE1275 routines are available for your 
> platform"));
>  }
> @@ -33,7 +33,8 @@ grub_install_register_ieee1275 (int is_prep, const char 
> *install_device,
>  void
>  grub_install_register_efi (grub_device_t efidir_grub_dev,
>                          const char *efifile_path,
> -                        const char *efi_distributor)
> +                        const char *efi_distributor,
> +                        int detect_nvram)
>  {
>    grub_util_error ("%s", _("no EFI routines are available for your 
> platform"));
>  }
> diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c
> index ca448bc11..4eb2c11c9 100644
> --- a/grub-core/osdep/unix/platform.c
> +++ b/grub-core/osdep/unix/platform.c
> @@ -134,7 +134,8 @@ grub_install_remove_efi_entries_by_distributor (const 
> char *efi_distributor)
>  int
>  grub_install_register_efi (grub_device_t efidir_grub_dev,
>                          const char *efifile_path,
> -                        const char *efi_distributor)
> +                        const char *efi_distributor,
> +                        int detect_nvram)
>  {
>    const char * efidir_disk;
>    int efidir_part;
> @@ -153,6 +154,21 @@ grub_install_register_efi (grub_device_t efidir_grub_dev,
>  #ifdef __linux__
>    grub_util_exec ((const char * []){ "modprobe", "-q", "efivars", NULL });
>  #endif
> +
> +  /* If requested, we try to detect if NVRAM access is available and if not,
> +     warn the user and resume normal operation.  */
> +  if (detect_nvram)
> +    {
> +      error = grub_util_exec_redirect_null ((const char * []){ "efibootmgr", 
> NULL });
> +      if (error == 2)
> +     {
> +       grub_util_warn ("%s", _("Auto-NVRAM selected and no EFI variable 
> support detected on the system."));

Wait, I have a feeling that you should fail hard here. In general
I think that if somebody passed --auto-nvram on platforms with NVRAM
and something fails then everything should fail. If somebody passed
--auto-nvram on platforms without NVRAM then any attempt to access
NVRAM should be skipped (silently?). Does it make sense?

Daniel