| Message ID | 1485803091-13678-1-git-send-email-93sam@debian.org |
|---|---|
| State | New |
| Headers | show |
Anybody interested in reviewing this? On Mon, Jan 30, 2017 at 07:04:51PM +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! > >Signed-off-by: Steve McIntyre <93sam@debian.org> >--- > grub-core/osdep/unix/platform.c | 24 +++++++++++++++--------- > include/grub/util/install.h | 2 +- > util/grub-install.c | 14 +++++++++++--- > 3 files changed, 27 insertions(+), 13 deletions(-) > >diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c >index 28cb37e..f9c376c 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 error = 0; > > if (!pid) > { > grub_util_warn (_("Unable to open stream from %s: %s"), > "efibootmgr", strerror (errno)); >- return; >+ return errno; > } > > FILE *fp = fdopen (fd, "r"); >@@ -98,7 +99,7 @@ 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); >@@ -119,23 +120,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", >+ error = grub_util_exec ((const char * []){ "efibootmgr", "-q", > "-b", bootnum, "-B", NULL }); > else >- grub_util_exec ((const char * []){ "efibootmgr", >+ error = grub_util_exec ((const char * []){ "efibootmgr", > "-b", bootnum, "-B", NULL }); > } > > free (line); >+ return error; > } > >-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 error = 0; > 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 +154,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); >+ error = grub_install_remove_efi_entries_by_distributor (efi_distributor); >+ if (error) >+ return error; > > char *efidir_part_str = xasprintf ("%d", efidir_part); > > if (!verbosity) >- grub_util_exec ((const char * []){ "efibootmgr", "-q", >+ error = 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", >+ error = 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 error; > } > > void >diff --git a/include/grub/util/install.h b/include/grub/util/install.h >index 9f517a1..58648e2 100644 >--- a/include/grub/util/install.h >+++ b/include/grub/util/install.h >@@ -209,7 +209,7 @@ grub_install_get_default_x86_platform (void); > const char * > grub_install_get_default_powerpc_machtype (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 d87d228..7a7e67e 100644 >--- a/util/grub-install.c >+++ b/util/grub-install.c >@@ -2002,9 +2002,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, >+ int error = 0; >+ error = grub_install_register_efi (efidir_grub_dev, > "\\System\\Library\\CoreServices", > efi_distributor); >+ if (error) >+ grub_util_error (_("efibootmgr failed to register the boot entry: %s"), >+ strerror (error)); > } > > grub_device_close (ins_dev); >@@ -2094,6 +2098,7 @@ main (int argc, char *argv[]) > { > char * efifile_path; > char * part; >+ int error = 0; > > /* Try to make this image bootable using the EFI Boot Manager, if available. */ > if (!efi_distributor || efi_distributor[0] == '\0') >@@ -2110,8 +2115,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); >+ error = grub_install_register_efi (efidir_grub_dev, >+ efifile_path, efi_distributor); >+ if (error) >+ grub_util_error (_("efibootmgr failed to register the boot entry: %s"), >+ strerror (error)); > } > break; > >-- >2.1.4 > > -- Steve McIntyre, Cambridge, UK. steve@einval.com Is there anybody out there? _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
30.01.2017 22:04, Steve McIntyre пишет: > 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! > This looks more or less cosmetic to me. First, errors are displayed to user so it is not that user is not aware. Second, efibootmgr is more or less optional. This is convenient but by far not the only one possibility to use newly installed grub. Third, even successful execution of efibootmgr does not mean it will actually boot grub - there are enough systems out there that will simply ditch grub entry and replace it with hard coded Windows one. So I'm fine with changing "no error reported" to "efibootmgr invocation failed; check your firmware settings" or similar, but I am not sure we need to abort grub-install in this case. What exact problem do you solve by aborting? > Signed-off-by: Steve McIntyre <93sam@debian.org> > --- > grub-core/osdep/unix/platform.c | 24 +++++++++++++++--------- > include/grub/util/install.h | 2 +- > util/grub-install.c | 14 +++++++++++--- > 3 files changed, 27 insertions(+), 13 deletions(-) > > diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c > index 28cb37e..f9c376c 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 error = 0; > > if (!pid) > { > grub_util_warn (_("Unable to open stream from %s: %s"), > "efibootmgr", strerror (errno)); > - return; > + return errno; > } > > FILE *fp = fdopen (fd, "r"); > @@ -98,7 +99,7 @@ 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); > @@ -119,23 +120,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", > + error = grub_util_exec ((const char * []){ "efibootmgr", "-q", > "-b", bootnum, "-B", NULL }); > else > - grub_util_exec ((const char * []){ "efibootmgr", > + error = grub_util_exec ((const char * []){ "efibootmgr", > "-b", bootnum, "-B", NULL }); > } > > free (line); > + return error; > } > > -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 error = 0; > 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 +154,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); > + error = grub_install_remove_efi_entries_by_distributor (efi_distributor); > + if (error) > + return error; > > char *efidir_part_str = xasprintf ("%d", efidir_part); > > if (!verbosity) > - grub_util_exec ((const char * []){ "efibootmgr", "-q", > + error = 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", > + error = 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 error; > } > > void > diff --git a/include/grub/util/install.h b/include/grub/util/install.h > index 9f517a1..58648e2 100644 > --- a/include/grub/util/install.h > +++ b/include/grub/util/install.h > @@ -209,7 +209,7 @@ grub_install_get_default_x86_platform (void); > const char * > grub_install_get_default_powerpc_machtype (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 d87d228..7a7e67e 100644 > --- a/util/grub-install.c > +++ b/util/grub-install.c > @@ -2002,9 +2002,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, > + int error = 0; > + error = grub_install_register_efi (efidir_grub_dev, > "\\System\\Library\\CoreServices", > efi_distributor); > + if (error) > + grub_util_error (_("efibootmgr failed to register the boot entry: %s"), > + strerror (error)); > } > > grub_device_close (ins_dev); > @@ -2094,6 +2098,7 @@ main (int argc, char *argv[]) > { > char * efifile_path; > char * part; > + int error = 0; > > /* Try to make this image bootable using the EFI Boot Manager, if available. */ > if (!efi_distributor || efi_distributor[0] == '\0') > @@ -2110,8 +2115,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); > + error = grub_install_register_efi (efidir_grub_dev, > + efifile_path, efi_distributor); > + if (error) > + grub_util_error (_("efibootmgr failed to register the boot entry: %s"), > + strerror (error)); > } > break; > >
On Thu, Feb 09, 2017 at 09:52:40PM +0300, Andrei Borzenkov wrote: >30.01.2017 22:04, Steve McIntyre пишет: >> 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! > >This looks more or less cosmetic to me. First, errors are displayed to >user so it is not that user is not aware. Maybe, maybe not - if this occurs in the middle of a set of package installations or upgrades on a system, text like this will get lost. >Second, efibootmgr is more or less optional. This is convenient but >by far not the only one possibility to use newly installed >grub. If you're running on a UEFI system, this is anything *but* optional. >Third, even successful execution of efibootmgr does not mean it will >actually boot grub - there are enough systems out there that will >simply ditch grub entry and replace it with hard coded Windows one. That's not an excuse for not catching errors on systems that *are* working. >So I'm fine with changing "no error reported" to "efibootmgr invocation >failed; check your firmware settings" or similar, but I am not sure we >need to abort grub-install in this case. What exact problem do you solve >by aborting? You pick up an error correctly, and report it upwards so that other programs calling grub-install can reliably check for errors, and maybe deal with those errors. I don't see why it's a problem to actually handle errors properly? In Debian we're seeing quite a few people reporting problems in this area. It would be better to catch and handle errors better here. See https://bugs.debian.org/852513 for an example.
On Thu, Feb 09, 2017 at 09:52:40PM +0300, Andrei Borzenkov wrote: > 30.01.2017 22:04, Steve McIntyre пишет: > > 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! > > This looks more or less cosmetic to me. First, errors are displayed to > user so it is not that user is not aware. I doubt most users pay any attention to what's printed during system installation if it finishes with a message saying it has completed successfully. Current grub-install behaviour prevents the installer system from automatically detecting when efibootmgr reports an error. > Second, efibootmgr is more or > less optional. This is convenient but by far not the only one > possibility to use newly installed grub. Sure, and you can install grub-bios by hex-editing a bootsector and dd:ing it onto the drive. That doesn't make it a relevant installation method. There is one relevant installation method for UEFI systems, and it depends on efibootmgr. > Third, even successful > execution of efibootmgr does not mean it will actually boot grub - there > are enough systems out there that will simply ditch grub entry and > replace it with hard coded Windows one. Not only is this factually incorrect (there is no hard-coded Windows entry, it's called the removable media path), it's beside the point. There are some broken systems. Which as it happens Steve has implemented support in Debian for dealing with. That does not mean that the mechanism that works on the vast majority of them, _and_ grub-install explicitly uses, should be considered some form of mythical creature. > So I'm fine with changing "no error reported" to "efibootmgr invocation > failed; check your firmware settings" or similar, but I am not sure we > need to abort grub-install in this case. What exact problem do you solve > by aborting? Giving the installation system the ability to automatically detect when the installation has verifiably failed and the system is left unbootable? I wouldn't have though that to be a controversial position. Regards, Leif > > Signed-off-by: Steve McIntyre <93sam@debian.org> > > --- > > grub-core/osdep/unix/platform.c | 24 +++++++++++++++--------- > > include/grub/util/install.h | 2 +- > > util/grub-install.c | 14 +++++++++++--- > > 3 files changed, 27 insertions(+), 13 deletions(-) > > > > diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c > > index 28cb37e..f9c376c 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 error = 0; > > > > if (!pid) > > { > > grub_util_warn (_("Unable to open stream from %s: %s"), > > "efibootmgr", strerror (errno)); > > - return; > > + return errno; > > } > > > > FILE *fp = fdopen (fd, "r"); > > @@ -98,7 +99,7 @@ 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); > > @@ -119,23 +120,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", > > + error = grub_util_exec ((const char * []){ "efibootmgr", "-q", > > "-b", bootnum, "-B", NULL }); > > else > > - grub_util_exec ((const char * []){ "efibootmgr", > > + error = grub_util_exec ((const char * []){ "efibootmgr", > > "-b", bootnum, "-B", NULL }); > > } > > > > free (line); > > + return error; > > } > > > > -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 error = 0; > > 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 +154,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); > > + error = grub_install_remove_efi_entries_by_distributor (efi_distributor); > > + if (error) > > + return error; > > > > char *efidir_part_str = xasprintf ("%d", efidir_part); > > > > if (!verbosity) > > - grub_util_exec ((const char * []){ "efibootmgr", "-q", > > + error = 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", > > + error = 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 error; > > } > > > > void > > diff --git a/include/grub/util/install.h b/include/grub/util/install.h > > index 9f517a1..58648e2 100644 > > --- a/include/grub/util/install.h > > +++ b/include/grub/util/install.h > > @@ -209,7 +209,7 @@ grub_install_get_default_x86_platform (void); > > const char * > > grub_install_get_default_powerpc_machtype (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 d87d228..7a7e67e 100644 > > --- a/util/grub-install.c > > +++ b/util/grub-install.c > > @@ -2002,9 +2002,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, > > + int error = 0; > > + error = grub_install_register_efi (efidir_grub_dev, > > "\\System\\Library\\CoreServices", > > efi_distributor); > > + if (error) > > + grub_util_error (_("efibootmgr failed to register the boot entry: %s"), > > + strerror (error)); > > } > > > > grub_device_close (ins_dev); > > @@ -2094,6 +2098,7 @@ main (int argc, char *argv[]) > > { > > char * efifile_path; > > char * part; > > + int error = 0; > > > > /* Try to make this image bootable using the EFI Boot Manager, if available. */ > > if (!efi_distributor || efi_distributor[0] == '\0') > > @@ -2110,8 +2115,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); > > + error = grub_install_register_efi (efidir_grub_dev, > > + efifile_path, efi_distributor); > > + if (error) > > + grub_util_error (_("efibootmgr failed to register the boot entry: %s"), > > + strerror (error)); > > } > > break; > > > > > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel
Hi folks, It would be nice to see this clear bug fixed for the upcoming release - any chance of getting it reviewed please? -- 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 28cb37e..f9c376c 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 error = 0; if (!pid) { grub_util_warn (_("Unable to open stream from %s: %s"), "efibootmgr", strerror (errno)); - return; + return errno; } FILE *fp = fdopen (fd, "r"); @@ -98,7 +99,7 @@ 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); @@ -119,23 +120,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", + error = grub_util_exec ((const char * []){ "efibootmgr", "-q", "-b", bootnum, "-B", NULL }); else - grub_util_exec ((const char * []){ "efibootmgr", + error = grub_util_exec ((const char * []){ "efibootmgr", "-b", bootnum, "-B", NULL }); } free (line); + return error; } -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 error = 0; 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 +154,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); + error = grub_install_remove_efi_entries_by_distributor (efi_distributor); + if (error) + return error; char *efidir_part_str = xasprintf ("%d", efidir_part); if (!verbosity) - grub_util_exec ((const char * []){ "efibootmgr", "-q", + error = 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", + error = 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 error; } void diff --git a/include/grub/util/install.h b/include/grub/util/install.h index 9f517a1..58648e2 100644 --- a/include/grub/util/install.h +++ b/include/grub/util/install.h @@ -209,7 +209,7 @@ grub_install_get_default_x86_platform (void); const char * grub_install_get_default_powerpc_machtype (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 d87d228..7a7e67e 100644 --- a/util/grub-install.c +++ b/util/grub-install.c @@ -2002,9 +2002,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, + int error = 0; + error = grub_install_register_efi (efidir_grub_dev, "\\System\\Library\\CoreServices", efi_distributor); + if (error) + grub_util_error (_("efibootmgr failed to register the boot entry: %s"), + strerror (error)); } grub_device_close (ins_dev); @@ -2094,6 +2098,7 @@ main (int argc, char *argv[]) { char * efifile_path; char * part; + int error = 0; /* Try to make this image bootable using the EFI Boot Manager, if available. */ if (!efi_distributor || efi_distributor[0] == '\0') @@ -2110,8 +2115,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); + error = grub_install_register_efi (efidir_grub_dev, + efifile_path, efi_distributor); + if (error) + grub_util_error (_("efibootmgr failed to register the boot entry: %s"), + strerror (error)); } 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! Signed-off-by: Steve McIntyre <93sam@debian.org> --- grub-core/osdep/unix/platform.c | 24 +++++++++++++++--------- include/grub/util/install.h | 2 +- util/grub-install.c | 14 +++++++++++--- 3 files changed, 27 insertions(+), 13 deletions(-) -- 2.1.4 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel