From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1cbWuq-0000yi-0E for mharc-grub-devel@gnu.org; Wed, 08 Feb 2017 13:24:28 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53688) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from <93sam@debian.org>) id 1cbUr3-0001LI-5D for grub-devel@gnu.org; Wed, 08 Feb 2017 11:12:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from <93sam@debian.org>) id 1cbUqy-0007g4-27 for grub-devel@gnu.org; Wed, 08 Feb 2017 11:12:25 -0500 Received: from cheddar.halon.org.uk ([93.93.131.118]:44069) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from <93sam@debian.org>) id 1cbUqx-0007ex-Nz for grub-devel@gnu.org; Wed, 08 Feb 2017 11:12:19 -0500 Received: from bsmtp by cheddar.halon.org.uk with local-bsmtp (Exim 4.84_2) (envelope-from <93sam@debian.org>) id 1cbUqo-0000Sl-Q8 for grub-devel@gnu.org; Wed, 08 Feb 2017 16:12:10 +0000 Received: from steve by tack.local with local (Exim 4.84_2) (envelope-from <93sam@debian.org>) id 1cbUqK-0000nW-Tq for grub-devel@gnu.org; Wed, 08 Feb 2017 16:11:40 +0000 Date: Wed, 8 Feb 2017 16:11:35 +0000 From: Steve McIntyre <93sam@debian.org> To: grub-devel@gnu.org Subject: Re: [PATCH] Make grub-install check for errors from efibootmgr Message-ID: <20170208161129.GB28152@einval.com> References: <1485803091-13678-1-git-send-email-93sam@debian.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1485803091-13678-1-git-send-email-93sam@debian.org> X-attached: none User-Agent: Mutt/1.5.23 (2014-03-12) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 93.93.131.118 X-Mailman-Approved-At: Wed, 08 Feb 2017 13:24:26 -0500 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Feb 2017 16:12:27 -0000 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?