| Message ID | 20180129185423.29681-1-steve@einval.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make grub-install check for errors from efibootmgr | expand |
On Mon, Jan 29, 2018 at 06:54:23PM +0000, Steve McIntyre wrote: > Code is currently ignoring errors from efibootmgr, giving users > clearly bogus output like: > > Setting up grub-efi-amd64 (2.02~beta3-4) ... > Installing for x86_64-efi platform. > Could not delete variable: No space left on device > Could not prepare Boot variable: No space left on device > Installation finished. No error reported. > > and then potentially unbootable systems. If efibootmgr fails, > grub-install should know that and report it! > > We've been using this patch in Debian now for some time, with no ill > effects. > > Signed-off-by: Steve McIntyre <93sam@debian.org> > --- > grub-core/osdep/unix/platform.c | 25 +++++++++++++++---------- > include/grub/util/install.h | 2 +- > util/grub-install.c | 18 +++++++++++++----- > 3 files changed, 29 insertions(+), 16 deletions(-) > > diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c > index a3fcfcaca..b3a617e44 100644 > --- a/grub-core/osdep/unix/platform.c > +++ b/grub-core/osdep/unix/platform.c > @@ -78,19 +78,20 @@ get_ofpathname (const char *dev) > dev); > } > > -static void > +static int > grub_install_remove_efi_entries_by_distributor (const char *efi_distributor) > { > int fd; > pid_t pid = grub_util_exec_pipe ((const char * []){ "efibootmgr", NULL }, &fd); > char *line = NULL; > size_t len = 0; > + int ret; > > if (!pid) > { > grub_util_warn (_("Unable to open stream from %s: %s"), > "efibootmgr", strerror (errno)); > - return; > + return errno; > } > > FILE *fp = fdopen (fd, "r"); > @@ -98,14 +99,13 @@ grub_install_remove_efi_entries_by_distributor (const char *efi_distributor) > { > grub_util_warn (_("Unable to open stream from %s: %s"), > "efibootmgr", strerror (errno)); > - return; > + return errno; > } > > line = xmalloc (80); > len = 80; > while (1) > { > - int ret; Oh, no, please do not do that here. If my first proposal conflicts with existing variables use second one. If second is bad please choose third. The rest of the patch LGTM. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On Tue, Jan 30, 2018 at 06:44:05PM +0100, Daniel Kiper wrote: >On Mon, Jan 29, 2018 at 06:54:23PM +0000, Steve McIntyre wrote: >> >> diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c >> index a3fcfcaca..b3a617e44 100644 >> --- a/grub-core/osdep/unix/platform.c >> +++ b/grub-core/osdep/unix/platform.c >> @@ -78,19 +78,20 @@ get_ofpathname (const char *dev) >> dev); >> } >> >> -static void >> +static int >> grub_install_remove_efi_entries_by_distributor (const char *efi_distributor) >> { >> int fd; >> pid_t pid = grub_util_exec_pipe ((const char * []){ "efibootmgr", NULL }, &fd); >> char *line = NULL; >> size_t len = 0; >> + int ret; >> >> if (!pid) >> { >> grub_util_warn (_("Unable to open stream from %s: %s"), >> "efibootmgr", strerror (errno)); >> - return; >> + return errno; >> } >> >> FILE *fp = fdopen (fd, "r"); >> @@ -98,14 +99,13 @@ grub_install_remove_efi_entries_by_distributor (const char *efi_distributor) >> { >> grub_util_warn (_("Unable to open stream from %s: %s"), >> "efibootmgr", strerror (errno)); >> - return; >> + return errno; >> } >> >> line = xmalloc (80); >> len = 80; >> while (1) >> { >> - int ret; > >Oh, no, please do not do that here. If my first proposal conflicts with >existing variables use second one. If second is bad please choose third. >The rest of the patch LGTM. OK, no problem. New version on its way in a sec. -- Steve McIntyre, Cambridge, UK. steve@einval.com You raise the blade, you make the change... You re-arrange me 'til I'm sane... _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c index a3fcfcaca..b3a617e44 100644 --- a/grub-core/osdep/unix/platform.c +++ b/grub-core/osdep/unix/platform.c @@ -78,19 +78,20 @@ get_ofpathname (const char *dev) dev); } -static void +static int grub_install_remove_efi_entries_by_distributor (const char *efi_distributor) { int fd; pid_t pid = grub_util_exec_pipe ((const char * []){ "efibootmgr", NULL }, &fd); char *line = NULL; size_t len = 0; + int ret; if (!pid) { grub_util_warn (_("Unable to open stream from %s: %s"), "efibootmgr", strerror (errno)); - return; + return errno; } FILE *fp = fdopen (fd, "r"); @@ -98,14 +99,13 @@ grub_install_remove_efi_entries_by_distributor (const char *efi_distributor) { grub_util_warn (_("Unable to open stream from %s: %s"), "efibootmgr", strerror (errno)); - return; + return errno; } line = xmalloc (80); len = 80; while (1) { - int ret; char *bootnum; ret = getline (&line, &len, fp); if (ret == -1) @@ -119,23 +119,25 @@ grub_install_remove_efi_entries_by_distributor (const char *efi_distributor) bootnum = line + sizeof ("Boot") - 1; bootnum[4] = '\0'; if (!verbosity) - grub_util_exec ((const char * []){ "efibootmgr", "-q", + ret = grub_util_exec ((const char * []){ "efibootmgr", "-q", "-b", bootnum, "-B", NULL }); else - grub_util_exec ((const char * []){ "efibootmgr", + ret = grub_util_exec ((const char * []){ "efibootmgr", "-b", bootnum, "-B", NULL }); } free (line); + return ret; } -void +int grub_install_register_efi (grub_device_t efidir_grub_dev, const char *efifile_path, const char *efi_distributor) { const char * efidir_disk; int efidir_part; + int ret; efidir_disk = grub_util_biosdisk_get_osdev (efidir_grub_dev->disk); efidir_part = efidir_grub_dev->disk->partition ? efidir_grub_dev->disk->partition->number + 1 : 1; @@ -151,23 +153,26 @@ grub_install_register_efi (grub_device_t efidir_grub_dev, grub_util_exec ((const char * []){ "modprobe", "-q", "efivars", NULL }); #endif /* Delete old entries from the same distributor. */ - grub_install_remove_efi_entries_by_distributor (efi_distributor); + ret = grub_install_remove_efi_entries_by_distributor (efi_distributor); + if (ret) + return ret; char *efidir_part_str = xasprintf ("%d", efidir_part); if (!verbosity) - grub_util_exec ((const char * []){ "efibootmgr", "-q", + ret = grub_util_exec ((const char * []){ "efibootmgr", "-q", "-c", "-d", efidir_disk, "-p", efidir_part_str, "-w", "-L", efi_distributor, "-l", efifile_path, NULL }); else - grub_util_exec ((const char * []){ "efibootmgr", + ret = grub_util_exec ((const char * []){ "efibootmgr", "-c", "-d", efidir_disk, "-p", efidir_part_str, "-w", "-L", efi_distributor, "-l", efifile_path, NULL }); free (efidir_part_str); + return ret; } void diff --git a/include/grub/util/install.h b/include/grub/util/install.h index 5910b0c09..0dba8b67f 100644 --- a/include/grub/util/install.h +++ b/include/grub/util/install.h @@ -210,7 +210,7 @@ grub_install_create_envblk_file (const char *name); const char * grub_install_get_default_x86_platform (void); -void +int grub_install_register_efi (grub_device_t efidir_grub_dev, const char *efifile_path, const char *efi_distributor); diff --git a/util/grub-install.c b/util/grub-install.c index 5e4cdfd2b..690f180c5 100644 --- a/util/grub-install.c +++ b/util/grub-install.c @@ -1848,9 +1848,13 @@ main (int argc, char *argv[]) if (!removable && update_nvram) { /* Try to make this image bootable using the EFI Boot Manager, if available. */ - grub_install_register_efi (efidir_grub_dev, - "\\System\\Library\\CoreServices", - efi_distributor); + int ret; + ret = grub_install_register_efi (efidir_grub_dev, + "\\System\\Library\\CoreServices", + efi_distributor); + if (ret) + grub_util_error (_("efibootmgr failed to register the boot entry: %s"), + strerror (ret)); } grub_device_close (ins_dev); @@ -1871,6 +1875,7 @@ main (int argc, char *argv[]) { char * efifile_path; char * part; + int ret; /* Try to make this image bootable using the EFI Boot Manager, if available. */ if (!efi_distributor || efi_distributor[0] == '\0') @@ -1887,8 +1892,11 @@ main (int argc, char *argv[]) efidir_grub_dev->disk->name, (part ? ",": ""), (part ? : "")); grub_free (part); - grub_install_register_efi (efidir_grub_dev, - efifile_path, efi_distributor); + ret = grub_install_register_efi (efidir_grub_dev, + efifile_path, efi_distributor); + if (ret) + grub_util_error (_("efibootmgr failed to register the boot entry: %s"), + strerror (ret)); } break;
Code is currently ignoring errors from efibootmgr, giving users clearly bogus output like: Setting up grub-efi-amd64 (2.02~beta3-4) ... Installing for x86_64-efi platform. Could not delete variable: No space left on device Could not prepare Boot variable: No space left on device Installation finished. No error reported. and then potentially unbootable systems. If efibootmgr fails, grub-install should know that and report it! We've been using this patch in Debian now for some time, with no ill effects. Signed-off-by: Steve McIntyre <93sam@debian.org> --- grub-core/osdep/unix/platform.c | 25 +++++++++++++++---------- include/grub/util/install.h | 2 +- util/grub-install.c | 18 +++++++++++++----- 3 files changed, 29 insertions(+), 16 deletions(-) -- 2.11.0 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel