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 <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;
> >  
> > 
> 
> 
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel