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