[PATCH] Make grub-install check for errors from efibootmgr

[PATCH] Make grub-install check for errors from efibootmgr

[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!

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             | 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 a3fcfcaca..fdd57a027 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 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..e783824ba 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,
+	      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);
@@ -1871,6 +1875,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')
@@ -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);
+	  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.11.0

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Daniel Kiper]

On Mon, Jan 29, 2018 at 02:04:54PM +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 | 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 a3fcfcaca..fdd57a027 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;

s/error/ret/ or s/error/rc/ or s/error/rv/ here and below please...
In that order of preference...

>    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 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..e783824ba 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,
> +	      int error = 0;

I think that you do not need this initialization.

Daniel

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Steve McIntyre]

On Mon, Jan 29, 2018 at 06:57:51PM +0100, Daniel Kiper wrote:
>On Mon, Jan 29, 2018 at 02:04:54PM +0000, Steve McIntyre wrote:
>>
>> diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c
>> index a3fcfcaca..fdd57a027 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;
>
>s/error/ret/ or s/error/rc/ or s/error/rv/ here and below please...
>In that order of preference...

ACK.

...

>> 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..e783824ba 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,
>> +	      int error = 0;
>
>I think that you do not need this initialization.

ACK. Updated patch coming shortly.

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
The two hard things in computing:
 * naming things
 * cache invalidation
 * off-by-one errors                  -- Stig Sandbeck Mathisen

[PATCH] Make grub-install check for errors from efibootmgr

[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!

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;
       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;
 
-- 
2.11.0

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Daniel Kiper]

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

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Steve McIntyre]

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...

[PATCH] Make grub-install check for errors from efibootmgr

[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!

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(-)

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;
 
-- 
2.11.0

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Daniel Kiper]

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

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Steve McIntyre]

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.

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Daniel Kiper]

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

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Steve McIntyre]

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...

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Leif Lindholm]

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

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Steve McIntyre]

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.

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
You raise the blade, you make the change... You re-arrange me 'til I'm sane...

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Andrei Borzenkov]

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;
>  
> 

Re: [PATCH] Make grub-install check for errors from efibootmgr

[Steve McIntyre]

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?

[PATCH] Make grub-install check for errors from efibootmgr

[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!

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