Make grub-install check for errors from efibootmgr

Message ID 1485803091-13678-1-git-send-email-93sam@debian.org
State New
Headers show

Commit Message

Steve McIntyre Jan. 30, 2017, 7:04 p.m.
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

Comments

Steve McIntyre Feb. 8, 2017, 4:11 p.m. | #1
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
Andrei Borzenkov Feb. 9, 2017, 6:52 p.m. | #2
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;
>  
>
Steve McIntyre Feb. 9, 2017, 8:37 p.m. | #3
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.
Leif Lindholm Feb. 11, 2017, 3:12 p.m. | #4
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
Steve McIntyre Feb. 24, 2017, 5:23 p.m. | #5
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

Patch hide | download patch | download mbox

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;