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 <address@hidden>
> ---
>  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