Make grub-install check for errors from efibootmgr

Message ID 20180131214936.19655-1-steve@einval.com
State Accepted
Commit 6400613ad0b463abc93362086a491cd2a5e99b0d
Headers show
Series
  • Make grub-install check for errors from efibootmgr
Related show

Commit Message

Steve McIntyre Jan. 31, 2018, 9:49 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!

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 | 24 +++++++++++++++---------
 include/grub/util/install.h     |  2 +-
 util/grub-install.c             | 18 +++++++++++++-----
 3 files changed, 29 insertions(+), 15 deletions(-)

-- 
2.11.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Comments

Daniel Kiper Feb. 6, 2018, 3:29 p.m. | #1
On Wed, Jan 31, 2018 at 09:49:36PM +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


s/this/similar/? If you are OK I will change that during commit.

> effects.

>

> Signed-off-by: Steve McIntyre <93sam@debian.org>


If there are no objections I will commit this in a week or so.

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Steve McIntyre Feb. 14, 2018, 3:30 p.m. | #2
Apologies for the delayed response - just found this in the archives
but for some reason I never got it in my inbox. Weird... :-/

On Tue, Feb 06, 2018 at 04:29:23PM +0100, Daniel Kiper wrote:
>On Wed, Jan 31, 2018 at 09:49:36PM +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

>

>s/this/similar/? If you are OK I will change that during commit.


Yup, perfect. :-)

>If there are no objections I will commit this in a week or so.


Please...

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
We don't need no education.
We don't need no thought control.


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
Daniel Kiper Feb. 14, 2018, 5:54 p.m. | #3
On Wed, Feb 14, 2018 at 03:30:48PM +0000, Steve McIntyre wrote:
> Apologies for the delayed response - just found this in the archives

> but for some reason I never got it in my inbox. Weird... :-/

>

> On Tue, Feb 06, 2018 at 04:29:23PM +0100, Daniel Kiper wrote:

> >On Wed, Jan 31, 2018 at 09:49:36PM +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

> >

> >s/this/similar/? If you are OK I will change that during commit.

>

> Yup, perfect. :-)

>

> >If there are no objections I will commit this in a week or so.

>

> Please...


Applied!

> --

> Steve McIntyre, Cambridge, UK.                                steve@einval.com

> We don't need no education.

> We don't need no thought control.


Ha! :-)))

Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Patch

diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c
index a3fcfcaca..ca448bc11 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 rc;
 
   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",
+	rc = grub_util_exec ((const char * []){ "efibootmgr", "-q",
 	      "-b", bootnum,  "-B", NULL });
       else
-	grub_util_exec ((const char * []){ "efibootmgr",
+	rc = grub_util_exec ((const char * []){ "efibootmgr",
 	      "-b", bootnum, "-B", NULL });
     }
 
   free (line);
+  return rc;
 }
 
-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 +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);
+  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;