Hey Daniel! Thank you for your feedback! I can drop the warning message, sure, I added it only for verbosity anyway so there's no real reason for keeping it around.
As for your second comment: generally this is what I want to achieve by checking the return value of the efibootmgr command. Whenever efibootmgr exits with an error code of 2 this basically means "EFI variables are not supported on this system." - i.e. that the system we're running on has no NVRAM (at least in efibootmgr's mind). In all other cases, if there is any other error returned by efiboomgr, fail the grub_install_register_efi() as we did before. Therefore if something fails on a platform with NVRAM, we fail hard as previously. Cheers, On 17 April 2018 at 20:23, Daniel Kiper <dki...@net-space.pl> wrote: > 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 -- Łukasz 'sil2100' Zemczak Foundations Team lukasz.zemc...@canonical.com www.canonical.com _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
