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 <address@hidden>
>---
> 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.                                address@hidden
Is there anybody out there?