Re: [PATCH v3 2/2] Minimise writes to EFI variable storage

[Steve McIntyre]

No individual comments, but... Overall, I'm thinking it would be nice
to see some more function-level comments to explain what's going on,
but that doesn't seem to match the prevailing style in the grub
source. :-/

On Sat, Mar 23, 2019 at 01:42:30PM +0000, Colin Watson wrote:
>Some UEFI firmware is easily provoked into running out of space in its
>variable storage.  This is usually due to certain kernel drivers (e.g.
>pstore), but regardless of the cause it can cause grub-install to fail
>because it currently asks efibootmgr to delete and re-add entries, and
>the deletion often doesn't result in an immediate garbage collection.
>Writing variables frequently also increases wear on the NVRAM which may
>have limited write cycles.  For these reasons, it's desirable to find a
>way to minimise writes while still allowing grub-install to ensure that
>a suitable boot entry exists.
>
>Unfortunately, efibootmgr doesn't offer an interface that would let
>grub-install do this.  It doesn't in general make very much effort to
>minimise writes; it doesn't allow modifying an existing Boot* variable
>entry, except in certain limited ways; and current versions don't have a
>way to export the expected variable data so that grub-install can
>compare it to the current data.  While it would be possible (and perhaps
>desirable?) to add at least some of this to efibootmgr, that would still
>leave the problem that there isn't a good upstreamable way for
>grub-install to guarantee that it has a new enough version of
>efibootmgr.  In any case, it's cumbersome and slow for grub-install to
>have to fork efibootmgr to get things done.
>
>Fortunately, a few years ago Peter Jones helpfully factored out a
>substantial part of efibootmgr to the efivar and efiboot libraries, and
>so it's now possible to have grub-install use those directly.  We still
>have to use some code from efibootmgr, but much less than would
>previously have been necessary.
>
>grub-install now reuses existing boot entries where possible, and avoids
>writing to variables when the new contents are the same as the old
>contents.  In the common upgrade case where nothing needs to change, it
>no longer writes to NVRAM at all.  It's also now slightly faster, since
>using libefivar is faster than forking efibootmgr.
>
>Fixes Debian bug #891434.
>
>Signed-off-by: Colin Watson <address@hidden>
>---
> INSTALL                         |   5 +
> Makefile.util.def               |  20 ++
> configure.ac                    |  12 +
> grub-core/osdep/efivar.c        |   3 +
> grub-core/osdep/unix/efivar.c   | 508 ++++++++++++++++++++++++++++++++
> grub-core/osdep/unix/platform.c | 100 +------
> include/grub/util/install.h     |   5 +
> util/grub-install.c             |   4 +-
> 8 files changed, 562 insertions(+), 95 deletions(-)
> create mode 100644 grub-core/osdep/efivar.c
> create mode 100644 grub-core/osdep/unix/efivar.c
>
>diff --git a/INSTALL b/INSTALL
>index 8acb40902..342c158e9 100644
>--- a/INSTALL
>+++ b/INSTALL
>@@ -41,6 +41,11 @@ configuring the GRUB.
> * Other standard GNU/Unix tools
> * a libc with large file support (e.g. glibc 2.1 or later)
> 
>+On Unix-based systems, you also need:
>+
>+* libefivar (recommended)
>+* libefiboot (recommended; your OS may ship this together with libefivar)
>+
> On GNU/Linux, you also need:
> 
> * libdevmapper 1.02.34 or later (recommended)
>diff --git a/Makefile.util.def b/Makefile.util.def
>index 969d32f00..60ec4d593 100644
>--- a/Makefile.util.def
>+++ b/Makefile.util.def
>@@ -535,6 +535,8 @@ program = {
>   common = grub-core/osdep/compress.c;
>   extra_dist = grub-core/osdep/unix/compress.c;
>   extra_dist = grub-core/osdep/basic/compress.c;
>+  common = grub-core/osdep/efivar.c;
>+  extra_dist = grub-core/osdep/unix/efivar.c;
>   common = util/editenv.c;
>   common = grub-core/osdep/blocklist.c;
>   common = grub-core/osdep/config.c;
>@@ -548,12 +550,15 @@ program = {
>   common = grub-core/kern/emu/argp_common.c;
>   common = grub-core/osdep/init.c;
> 
>+  cflags = '$(EFIVAR_CFLAGS)';
>+
>   ldadd = '$(LIBLZMA)';
>   ldadd = libgrubmods.a;
>   ldadd = libgrubgcry.a;
>   ldadd = libgrubkern.a;
>   ldadd = grub-core/lib/gnulib/libgnu.a;
>   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) 
> $(LIBGEOM)';
>+  ldadd = '$(EFIVAR_LIBS)';
> 
>   condition = COND_HAVE_EXEC;
> };
>@@ -582,6 +587,8 @@ program = {
>   extra_dist = grub-core/osdep/basic/no_platform.c;
>   extra_dist = grub-core/osdep/unix/platform.c;
>   common = grub-core/osdep/compress.c;
>+  common = grub-core/osdep/efivar.c;
>+  extra_dist = grub-core/osdep/unix/efivar.c;
>   common = util/editenv.c;
>   common = grub-core/osdep/blocklist.c;
>   common = grub-core/osdep/config.c;
>@@ -595,12 +602,15 @@ program = {
>   common = grub-core/kern/emu/argp_common.c;
>   common = grub-core/osdep/init.c;
> 
>+  cflags = '$(EFIVAR_CFLAGS)';
>+
>   ldadd = '$(LIBLZMA)';
>   ldadd = libgrubmods.a;
>   ldadd = libgrubgcry.a;
>   ldadd = libgrubkern.a;
>   ldadd = grub-core/lib/gnulib/libgnu.a;
>   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) 
> $(LIBGEOM)';
>+  ldadd = '$(EFIVAR_LIBS)';
> };
> 
> program = {
>@@ -622,6 +632,8 @@ program = {
>   common = grub-core/osdep/platform.c;
>   common = grub-core/osdep/platform_unix.c;
>   common = grub-core/osdep/compress.c;
>+  common = grub-core/osdep/efivar.c;
>+  extra_dist = grub-core/osdep/unix/efivar.c;
>   common = util/editenv.c;
>   common = grub-core/osdep/blocklist.c;
>   common = grub-core/osdep/config.c;
>@@ -634,12 +646,15 @@ program = {
>   common = grub-core/kern/emu/argp_common.c;
>   common = grub-core/osdep/init.c;
> 
>+  cflags = '$(EFIVAR_CFLAGS)';
>+
>   ldadd = '$(LIBLZMA)';
>   ldadd = libgrubmods.a;
>   ldadd = libgrubgcry.a;
>   ldadd = libgrubkern.a;
>   ldadd = grub-core/lib/gnulib/libgnu.a;
>   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) 
> $(LIBGEOM)';
>+  ldadd = '$(EFIVAR_LIBS)';
> };
> 
> program = {
>@@ -661,6 +676,8 @@ program = {
>   common = grub-core/osdep/platform.c;
>   common = grub-core/osdep/platform_unix.c;
>   common = grub-core/osdep/compress.c;
>+  common = grub-core/osdep/efivar.c;
>+  extra_dist = grub-core/osdep/unix/efivar.c;
>   common = util/editenv.c;
>   common = grub-core/osdep/blocklist.c;
>   common = grub-core/osdep/config.c;
>@@ -670,12 +687,15 @@ program = {
>   common = grub-core/kern/emu/argp_common.c;
>   common = grub-core/osdep/init.c;
> 
>+  cflags = '$(EFIVAR_CFLAGS)';
>+
>   ldadd = '$(LIBLZMA)';
>   ldadd = libgrubmods.a;
>   ldadd = libgrubgcry.a;
>   ldadd = libgrubkern.a;
>   ldadd = grub-core/lib/gnulib/libgnu.a;
>   ldadd = '$(LIBINTL) $(LIBDEVMAPPER) $(LIBUTIL) $(LIBZFS) $(LIBNVPAIR) 
> $(LIBGEOM)';
>+  ldadd = '$(EFIVAR_LIBS)';
> };
> 
> script = {
>diff --git a/configure.ac b/configure.ac
>index 05d040cd6..2add2a092 100644
>--- a/configure.ac
>+++ b/configure.ac
>@@ -443,6 +443,18 @@ AC_CHECK_HEADER([util.h], [
> ])
> AC_SUBST([LIBUTIL])
> 
>+case "$host_os" in
>+  cygwin | windows* | mingw32* | aros*)
>+    ;;
>+  *)
>+    # For setting EFI variables in grub-install.
>+    PKG_CHECK_MODULES([EFIVAR], [efivar efiboot], [
>+      AC_DEFINE([HAVE_EFIVAR], [1],
>+                [Define to 1 if you have the efivar and efiboot libraries.])
>+    ], [:])
>+    ;;
>+esac
>+
> AC_CACHE_CHECK([whether -Wtrampolines work], [grub_cv_host_cc_wtrampolines], [
>   SAVED_CFLAGS="$CFLAGS"
>   CFLAGS="$HOST_CFLAGS -Wtrampolines -Werror"
>diff --git a/grub-core/osdep/efivar.c b/grub-core/osdep/efivar.c
>new file mode 100644
>index 000000000..d2750e252
>--- /dev/null
>+++ b/grub-core/osdep/efivar.c
>@@ -0,0 +1,3 @@
>+#if !defined (__MINGW32__) && !defined (__CYGWIN__) && !defined (__AROS__)
>+#include "unix/efivar.c"
>+#endif
>diff --git a/grub-core/osdep/unix/efivar.c b/grub-core/osdep/unix/efivar.c
>new file mode 100644
>index 000000000..4a58328b4
>--- /dev/null
>+++ b/grub-core/osdep/unix/efivar.c
>@@ -0,0 +1,508 @@
>+/*
>+ *  GRUB  --  GRand Unified Bootloader
>+ *  Copyright (C) 2013,2019 Free Software Foundation, Inc.
>+ *
>+ *  GRUB is free software: you can redistribute it and/or modify
>+ *  it under the terms of the GNU General Public License as published by
>+ *  the Free Software Foundation, either version 3 of the License, or
>+ *  (at your option) any later version.
>+ *
>+ *  GRUB is distributed in the hope that it will be useful,
>+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+ *  GNU General Public License for more details.
>+ *
>+ *  You should have received a copy of the GNU General Public License
>+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>+ */
>+
>+/* Contains portions derived from efibootmgr, licensed as follows:
>+ *
>+ *  Copyright (C) 2001-2004 Dell, Inc. <address@hidden>
>+ *  Copyright 2015-2016 Red Hat, Inc. <address@hidden>
>+ *
>+ *  This program is free software; you can redistribute it and/or modify
>+ *  it under the terms of the GNU General Public License as published by
>+ *  the Free Software Foundation; either version 2 of the License, or
>+ *  (at your option) any later version.
>+ */
>+
>+#include <config.h>
>+
>+#ifdef HAVE_EFIVAR
>+
>+#include <grub/util/install.h>
>+#include <grub/emu/hostdisk.h>
>+#include <grub/util/misc.h>
>+#include <grub/list.h>
>+#include <grub/misc.h>
>+#include <grub/emu/exec.h>
>+#include <sys/types.h>
>+#include <ctype.h>
>+#include <errno.h>
>+#include <stdlib.h>
>+#include <string.h>
>+
>+#include <efiboot.h>
>+#include <efivar.h>
>+
>+struct efi_variable {
>+  struct efi_variable *next;
>+  struct efi_variable **prev;
>+  char *name;
>+  efi_guid_t guid;
>+  uint8_t *data;
>+  size_t data_size;
>+  uint32_t attributes;
>+  int num;
>+};
>+
>+/* Boot option attributes.  */
>+#define LOAD_OPTION_ACTIVE 0x00000001
>+
>+/* GUIDs.  */
>+#define BLKX_UNKNOWN_GUID \
>+  EFI_GUID (0x47c7b225, 0xc42a, 0x11d2, 0x8e57, 0x00, 0xa0, 0xc9, 0x69, \
>+          0x72, 0x3b)
>+
>+/* Log all errors recorded by libefivar/libefiboot.  */
>+static void
>+show_efi_errors (void)
>+{
>+  int i;
>+  int saved_errno = errno;
>+
>+  for (i = 0; ; ++i)
>+    {
>+      char *filename, *function, *message = NULL;
>+      int line, error = 0, rc;
>+
>+      rc = efi_error_get (i, &filename, &function, &line, &message, &error);
>+      if (rc < 0)
>+      /* Give up.  The caller is going to log an error anyway.  */
>+      break;
>+      if (rc == 0)
>+      /* No more errors.  */
>+      break;
>+      grub_util_warn ("%s: %s: %s", function, message, strerror (error));
>+    }
>+
>+  efi_error_clear ();
>+  errno = saved_errno;
>+}
>+
>+static struct efi_variable *
>+new_efi_variable (void)
>+{
>+  struct efi_variable *new = xmalloc (sizeof (*new));
>+  memset (new, 0, sizeof (*new));
>+  return new;
>+}
>+
>+static struct efi_variable *
>+new_boot_variable (void)
>+{
>+  struct efi_variable *new = new_efi_variable ();
>+  new->guid = EFI_GLOBAL_GUID;
>+  new->attributes = EFI_VARIABLE_NON_VOLATILE |
>+                  EFI_VARIABLE_BOOTSERVICE_ACCESS |
>+                  EFI_VARIABLE_RUNTIME_ACCESS;
>+  return new;
>+}
>+
>+static void
>+free_efi_variable (struct efi_variable *entry)
>+{
>+  if (entry)
>+    {
>+      free (entry->name);
>+      free (entry->data);
>+      free (entry);
>+    }
>+}
>+
>+static int
>+read_efi_variable (const char *name, struct efi_variable **entry)
>+{
>+  struct efi_variable *new = new_efi_variable ();
>+  int rc;
>+
>+  rc = efi_get_variable (EFI_GLOBAL_GUID, name,
>+                       &new->data, &new->data_size, &new->attributes);
>+  if (rc < 0)
>+    {
>+      free_efi_variable (new);
>+      new = NULL;
>+    }
>+
>+  if (new)
>+    {
>+      /* Latest Apple firmware sets the high bit which appears invalid
>+       to the Linux kernel if we write it back, so let's zero it out if it
>+       is set since it would be invalid to set it anyway.  */
>+      new->attributes = new->attributes & ~(1 << 31);
>+
>+      new->name = xstrdup (name);
>+      new->guid = EFI_GLOBAL_GUID;
>+    }
>+
>+  *entry = new;
>+  return rc;
>+}
>+
>+/* Set an EFI variable, but only if it differs from the current value.
>+   Some firmware implementations are liable to fill up flash space if we set
>+   variables unnecessarily, so try to keep write activity to a minimum. */
>+static int
>+set_efi_variable (const char *name, struct efi_variable *entry)
>+{
>+  struct efi_variable *old = NULL;
>+  int rc = 0;
>+
>+  read_efi_variable (name, &old);
>+  efi_error_clear ();
>+  if (old && old->attributes == entry->attributes &&
>+      old->data_size == entry->data_size &&
>+      memcmp (old->data, entry->data, entry->data_size) == 0)
>+    grub_util_info ("skipping unnecessary update of EFI variable %s", name);
>+  else
>+    {
>+      rc = efi_set_variable (EFI_GLOBAL_GUID, name,
>+                           entry->data, entry->data_size, entry->attributes,
>+                           0644);
>+      if (rc < 0)
>+      grub_util_warn (_("Cannot set EFI variable %s"), name);
>+    }
>+  free_efi_variable (old);
>+  return rc;
>+}
>+
>+static int
>+cmpvarbyname (const void *p1, const void *p2)
>+{
>+  const struct efi_variable *var1 = *(const struct efi_variable **)p1;
>+  const struct efi_variable *var2 = *(const struct efi_variable **)p2;
>+  return strcmp (var1->name, var2->name);
>+}
>+
>+static int
>+read_boot_variables (struct efi_variable **varlist)
>+{
>+  int rc;
>+  efi_guid_t *guid = NULL;
>+  char *name = NULL;
>+  struct efi_variable **newlist = NULL;
>+  int nentries = 0;
>+  int i;
>+
>+  while ((rc = efi_get_next_variable_name (&guid, &name)) > 0)
>+    {
>+      const char *snum = name + sizeof ("Boot") - 1;
>+      struct efi_variable *var = NULL;
>+      unsigned int num;
>+
>+      if (memcmp (guid, &efi_guid_global, sizeof (efi_guid_global)) != 0 ||
>+        strncmp (name, "Boot", sizeof ("Boot") - 1) != 0 ||
>+        !grub_isxdigit (snum[0]) || !grub_isxdigit (snum[1]) ||
>+        !grub_isxdigit (snum[2]) || !grub_isxdigit (snum[3]))
>+      continue;
>+
>+      rc = read_efi_variable (name, &var);
>+      if (rc < 0)
>+      break;
>+
>+      if (sscanf (var->name, "Boot%04X-%*s", &num) == 1 && num < 65536)
>+      var->num = num;
>+
>+      newlist = xrealloc (newlist, (++nentries) * sizeof (*newlist));
>+      newlist[nentries - 1] = var;
>+    }
>+  if (rc == 0 && newlist)
>+    {
>+      qsort (newlist, nentries, sizeof (*newlist), cmpvarbyname);
>+      for (i = nentries - 1; i >= 0; --i)
>+      grub_list_push (GRUB_AS_LIST_P (varlist), GRUB_AS_LIST (newlist[i]));
>+    }
>+  else if (newlist)
>+    {
>+      for (i = 0; i < nentries; ++i)
>+      free_efi_variable (newlist[i]);
>+      free (newlist);
>+    }
>+  return rc;
>+}
>+
>+#define GET_ORDER(data, i) \
>+  ((uint16_t) ((data)[(i) * 2]) + ((data)[(i) * 2 + 1] << 8))
>+#define SET_ORDER(data, i, num) \
>+  do { \
>+    (data)[(i) * 2] = (num) & 0xFF; \
>+    (data)[(i) * 2 + 1] = ((num) >> 8) & 0xFF; \
>+  } while (0)
>+
>+static void
>+remove_from_boot_order (struct efi_variable *order, uint16_t num)
>+{
>+  unsigned int old_i, new_i;
>+
>+  /* We've got an array (in order->data) of the order.  Squeeze out any
>+     instance of the entry we're deleting by shifting the remainder down.  */
>+  for (old_i = 0, new_i = 0;
>+       old_i < order->data_size / sizeof (uint16_t);
>+       ++old_i)
>+    {
>+      uint16_t old_num = GET_ORDER (order->data, old_i);
>+      if (old_num != num)
>+      {
>+        if (new_i != old_i)
>+          SET_ORDER (order->data, new_i, old_num);
>+        ++new_i;
>+      }
>+    }
>+
>+  order->data_size = new_i * sizeof (uint16_t);
>+}
>+
>+static void
>+add_to_boot_order (struct efi_variable *order, uint16_t num)
>+{
>+  int i;
>+  size_t new_data_size;
>+  uint8_t *new_data;
>+
>+  /* Check whether this entry is already in the boot order.  If it is, leave
>+     it alone.  */
>+  for (i = 0; i < order->data_size / sizeof (uint16_t); ++i)
>+    if (GET_ORDER (order->data, i) == num)
>+      return;
>+
>+  new_data_size = order->data_size + sizeof (uint16_t);
>+  new_data = xmalloc (new_data_size);
>+  SET_ORDER (new_data, 0, num);
>+  memcpy (new_data + sizeof (uint16_t), order->data, order->data_size);
>+  free (order->data);
>+  order->data = new_data;
>+  order->data_size = new_data_size;
>+}
>+
>+static int
>+find_free_boot_num (struct efi_variable *entries)
>+{
>+  int num_vars = 0, i;
>+  struct efi_variable *entry;
>+
>+  FOR_LIST_ELEMENTS (entry, entries)
>+    ++num_vars;
>+
>+  if (num_vars == 0)
>+    return 0;
>+
>+  /* O(n^2), but n is small and this is easy. */
>+  for (i = 0; i < num_vars; ++i)
>+    {
>+      int found = 0;
>+      FOR_LIST_ELEMENTS (entry, entries)
>+      {
>+        if (entry->num == i)
>+          {
>+            found = 1;
>+            break;
>+          }
>+      }
>+      if (!found)
>+      return i;
>+    }
>+
>+  return i;
>+}
>+
>+static int
>+get_edd_version (void)
>+{
>+  efi_guid_t blkx_guid = BLKX_UNKNOWN_GUID;
>+  uint8_t *data = NULL;
>+  size_t data_size = 0;
>+  uint32_t attributes;
>+  efidp_header *path;
>+  int rc;
>+
>+  rc = efi_get_variable (blkx_guid, "blk0", &data, &data_size, &attributes);
>+  if (rc < 0)
>+    return rc;
>+
>+  path = (efidp_header *) data;
>+  if (path->type == 2 && path->subtype == 1)
>+    return 3;
>+  return 1;
>+}
>+
>+static struct efi_variable *
>+make_boot_variable (int num, const char *disk, int part, const char *loader,
>+                  const char *label)
>+{
>+  struct efi_variable *entry = new_boot_variable ();
>+  uint32_t options;
>+  uint32_t edd10_devicenum;
>+  ssize_t dp_needed, loadopt_needed;
>+  efidp dp = NULL;
>+
>+  options = EFIBOOT_ABBREV_HD;
>+  switch (get_edd_version ()) {
>+    case 1:
>+      options = EFIBOOT_ABBREV_EDD10;
>+      break;
>+    case 3:
>+      options = EFIBOOT_ABBREV_NONE;
>+      break;
>+  }
>+
>+  /* This may not be the right disk; but it's probably only an issue on very
>+     old hardware anyway. */
>+  edd10_devicenum = 0x80;
>+
>+  dp_needed = efi_generate_file_device_path_from_esp (NULL, 0, disk, part,
>+                                                    loader, options,
>+                                                    edd10_devicenum);
>+  if (dp_needed < 0)
>+    goto err;
>+
>+  dp = xmalloc (dp_needed);
>+  dp_needed = efi_generate_file_device_path_from_esp ((uint8_t *) dp,
>+                                                    dp_needed, disk, part,
>+                                                    loader, options,
>+                                                    edd10_devicenum);
>+  if (dp_needed < 0)
>+    goto err;
>+
>+  loadopt_needed = efi_loadopt_create (NULL, 0, LOAD_OPTION_ACTIVE,
>+                                     dp, dp_needed, (unsigned char *) label,
>+                                     NULL, 0);
>+  if (loadopt_needed < 0)
>+    goto err;
>+  entry->data_size = loadopt_needed;
>+  entry->data = xmalloc (entry->data_size);
>+  loadopt_needed = efi_loadopt_create (entry->data, entry->data_size,
>+                                     LOAD_OPTION_ACTIVE, dp, dp_needed,
>+                                     (unsigned char *) label, NULL, 0);
>+  if (loadopt_needed < 0)
>+    goto err;
>+
>+  entry->name = xasprintf ("Boot%04X", num);
>+  entry->num = num;
>+
>+  return entry;
>+
>+err:
>+  free_efi_variable (entry);
>+  free (dp);
>+  return NULL;
>+}
>+
>+int
>+grub_install_efivar_register_efi (grub_device_t efidir_grub_dev,
>+                                const char *efifile_path,
>+                                const char *efi_distributor)
>+{
>+  const char *efidir_disk;
>+  int efidir_part;
>+  struct efi_variable *entries = NULL, *entry;
>+  struct efi_variable *order;
>+  int entry_num = -1;
>+  int rc;
>+
>+  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;
>+
>+#ifdef __linux__
>+  /*
>+   * Linux uses efivarfs (mounted on /sys/firmware/efi/efivars) to access the
>+   * EFI variable store. Some legacy systems may still use the deprecated
>+   * efivars interface (accessed through /sys/firmware/efi/vars). Where both
>+   * are present, libefivar will use the former in preference, so attempting
>+   * to load efivars will not interfere with later operations.
>+   */
>+  grub_util_exec_redirect_all ((const char * []){ "modprobe", "efivars", NULL 
>},
>+                             NULL, NULL, "/dev/null");
>+#endif
>+
>+  if (!efi_variables_supported ())
>+    {
>+      grub_util_warn ("%s",
>+                    _("EFI variables are not supported on this system."));
>+      /* Let the user continue.  Perhaps they can still arrange to boot GRUB
>+         manually.  */
>+      return 0;
>+    }
>+
>+  rc = read_boot_variables (&entries);
>+  if (rc < 0)
>+    {
>+      grub_util_warn ("%s", _("Cannot read EFI Boot* variables"));
>+      goto err;
>+    }
>+  rc = read_efi_variable ("BootOrder", &order);
>+  if (rc < 0)
>+    {
>+      order = new_boot_variable ();
>+      order->name = xstrdup ("BootOrder");
>+      efi_error_clear ();
>+    }
>+
>+  /* Delete old entries from the same distributor.  */
>+  FOR_LIST_ELEMENTS (entry, entries)
>+    {
>+      efi_load_option *load_option = (efi_load_option *) entry->data;
>+      const char *label;
>+
>+      if (entry->num < 0)
>+      continue;
>+      label = (const char *) efi_loadopt_desc (load_option, entry->data_size);
>+      if (strcasecmp (label, efi_distributor) != 0)
>+      continue;
>+
>+      /* To avoid problems with some firmware implementations, reuse the first
>+         matching variable we find rather than deleting and recreating it.  */
>+      if (entry_num == -1)
>+      entry_num = entry->num;
>+      else
>+      {
>+        grub_util_info ("deleting superfluous EFI variable %s (%s)",
>+                        entry->name, label);
>+        rc = efi_del_variable (EFI_GLOBAL_GUID, entry->name);
>+        if (rc < 0)
>+          {
>+            grub_util_warn (_("Cannot delete EFI variable %s"), entry->name);
>+            goto err;
>+          }
>+      }
>+
>+      remove_from_boot_order (order, (uint16_t) entry->num);
>+    }
>+
>+  if (entry_num == -1)
>+    entry_num = find_free_boot_num (entries);
>+  entry = make_boot_variable (entry_num, efidir_disk, efidir_part,
>+                            efifile_path, efi_distributor);
>+  if (!entry)
>+    goto err;
>+
>+  grub_util_info ("setting EFI variable %s", entry->name);
>+  rc = set_efi_variable (entry->name, entry);
>+  if (rc < 0)
>+    goto err;
>+
>+  add_to_boot_order (order, (uint16_t) entry_num);
>+
>+  grub_util_info ("setting EFI variable BootOrder");
>+  rc = set_efi_variable ("BootOrder", order);
>+  if (rc < 0)
>+    goto err;
>+
>+  return 0;
>+
>+err:
>+  show_efi_errors ();
>+  return errno;
>+}
>+
>+#endif /* HAVE_EFIVAR */
>diff --git a/grub-core/osdep/unix/platform.c b/grub-core/osdep/unix/platform.c
>index 55b8f4016..9d4530a38 100644
>--- a/grub-core/osdep/unix/platform.c
>+++ b/grub-core/osdep/unix/platform.c
>@@ -19,15 +19,12 @@
> #include <config.h>
> 
> #include <grub/util/install.h>
>-#include <grub/emu/hostdisk.h>
> #include <grub/util/misc.h>
> #include <grub/misc.h>
> #include <grub/i18n.h>
> #include <grub/emu/exec.h>
> #include <sys/types.h>
>-#include <dirent.h>
> #include <string.h>
>-#include <errno.h>
> 
> static char *
> get_ofpathname (const char *dev)
>@@ -78,102 +75,19 @@ get_ofpathname (const char *dev)
>                  dev);
> }
> 
>-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 rc = 0;
>-
>-  if (!pid)
>-    {
>-      grub_util_warn (_("Unable to open stream from %s: %s"),
>-                    "efibootmgr", strerror (errno));
>-      return errno;
>-    }
>-
>-  FILE *fp = fdopen (fd, "r");
>-  if (!fp)
>-    {
>-      grub_util_warn (_("Unable to open stream from %s: %s"),
>-                    "efibootmgr", strerror (errno));
>-      return errno;
>-    }
>-
>-  line = xmalloc (80);
>-  len = 80;
>-  while (1)
>-    {
>-      int ret;
>-      char *bootnum;
>-      ret = getline (&line, &len, fp);
>-      if (ret == -1)
>-      break;
>-      if (grub_memcmp (line, "Boot", sizeof ("Boot") - 1) != 0
>-        || line[sizeof ("Boot") - 1] < '0'
>-        || line[sizeof ("Boot") - 1] > '9')
>-      continue;
>-      if (!strcasestr (line, efi_distributor))
>-      continue;
>-      bootnum = line + sizeof ("Boot") - 1;
>-      bootnum[4] = '\0';
>-      if (!verbosity)
>-      rc = grub_util_exec ((const char * []){ "efibootmgr", "-q",
>-            "-b", bootnum,  "-B", NULL });
>-      else
>-      rc = grub_util_exec ((const char * []){ "efibootmgr",
>-            "-b", bootnum, "-B", NULL });
>-    }
>-
>-  free (line);
>-  return rc;
>-}
>-
> 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 ret;
>-  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;
>-
>-  if (grub_util_exec_redirect_null ((const char * []){ "efibootmgr", 
>"--version", NULL }))
>-    {
>-      /* TRANSLATORS: This message is shown when required executable `%s'
>-       isn't found.  */
>-      grub_util_error (_("%s: not found"), "efibootmgr");
>-    }
>-
>-  /* On Linux, we need the efivars kernel modules.  */
>-#ifdef __linux__
>-  grub_util_exec ((const char * []){ "modprobe", "-q", "efivars", NULL });
>+#ifdef HAVE_EFIVAR
>+  return grub_install_efivar_register_efi (efidir_grub_dev, efifile_path,
>+                                         efi_distributor);
>+#else
>+  grub_util_error ("%s",
>+                 _("GRUB was not built with efivar support; "
>+                   "cannot register EFI boot entry"));
> #endif
>-  /* Delete old entries from the same distributor.  */
>-  ret = grub_install_remove_efi_entries_by_distributor (efi_distributor);
>-  if (ret)
>-    return ret;
>-
>-  char *efidir_part_str = xasprintf ("%d", efidir_part);
>-
>-  if (!verbosity)
>-    ret = grub_util_exec ((const char * []){ "efibootmgr", "-q",
>-        "-c", "-d", efidir_disk,
>-        "-p", efidir_part_str, "-w",
>-        "-L", efi_distributor, "-l", 
>-        efifile_path, NULL });
>-  else
>-    ret = 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 ret;
> }
> 
> void
>diff --git a/include/grub/util/install.h b/include/grub/util/install.h
>index 2631b1074..dcad16ac5 100644
>--- a/include/grub/util/install.h
>+++ b/include/grub/util/install.h
>@@ -216,6 +216,11 @@ grub_install_get_default_arm_platform (void);
> const char *
> grub_install_get_default_x86_platform (void);
> 
>+int
>+grub_install_efivar_register_efi (grub_device_t efidir_grub_dev,
>+                                const char *efifile_path,
>+                                const char *efi_distributor);
>+
> int
> grub_install_register_efi (grub_device_t efidir_grub_dev,
>                          const char *efifile_path,
>diff --git a/util/grub-install.c b/util/grub-install.c
>index 264f9ecdc..a61df056e 100644
>--- a/util/grub-install.c
>+++ b/util/grub-install.c
>@@ -1885,7 +1885,7 @@ main (int argc, char *argv[])
>                                              
> "\\System\\Library\\CoreServices",
>                                              efi_distributor);
>             if (ret)
>-              grub_util_error (_("efibootmgr failed to register the boot 
>entry: %s"),
>+              grub_util_error (_("failed to register the EFI boot entry: %s"),
>                                strerror (ret));
>           }
> 
>@@ -1929,7 +1929,7 @@ main (int argc, char *argv[])
>         ret = grub_install_register_efi (efidir_grub_dev,
>                                          efifile_path, efi_distributor);
>         if (ret)
>-          grub_util_error (_("efibootmgr failed to register the boot entry: 
>%s"),
>+          grub_util_error (_("failed to register the EFI boot entry: %s"),
>                            strerror (ret));
>       }
>       break;

Reviewed-by: Steve McIntyre <address@hidden>

-- 
Steve McIntyre, Cambridge, UK.                                address@hidden
< Aardvark> I dislike C++ to start with. C++11 just seems to be
            handing rope-creating factories for users to hang multiple
            instances of themselves.