Re: [PATCH 4/4] Update grub editenv to support modifying envblk

[Daniel Kiper]

On Tue, Feb 25, 2020 at 11:26:47AM -0800, Paul Dagnelie wrote:
> This patch adds the capability for the editenv utility to modify the
> envblk, and adds ZFS-specific handlers that will be built if GRUB is
> built with libzfs support. It also adds logic that editenv uses to
> detect GRUB's (root) filesystem.
>
> One question I have related to this patch is if there is some existing
> requirement on the minimum version of ZFS being used to build GRUB; if

I do not know about any of such requirements.

> not, this code probably needs some additional checks to verify that
> the attached version of libzfs has the zpool_set_bootenv and
> zpool_get_bootenv functions.

This check should happen when configure runs.

> commit 98f854215f7e0488cbe2a639b8dde76015c26e55
> Author:     Paul Dagnelie <address@hidden>
> AuthorDate: Mon Feb 24 14:29:47 2020 -0800
> Commit:     Paul Dagnelie <address@hidden>
> CommitDate: Tue Feb 25 10:08:08 2020 -0800
>
>     Update editenv to support editing zfs envblock
> ---
>  include/grub/util/libzfs.h |   7 ++
>  util/grub-editenv.c        | 279 
> +++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 277 insertions(+), 9 deletions(-)
>
> diff --git a/include/grub/util/libzfs.h b/include/grub/util/libzfs.h
> index a02caa335..9990e8120 100644
> --- a/include/grub/util/libzfs.h
> +++ b/include/grub/util/libzfs.h
> @@ -40,6 +40,13 @@ extern int zpool_get_physpath (zpool_handle_t *,
> const char *);
>
>  extern nvlist_t *zpool_get_config (zpool_handle_t *, nvlist_t **);
>
> +extern libzfs_handle_t *zpool_get_handle(zpool_handle_t *);
> +
> +extern int zpool_set_bootenv(zpool_handle_t *, const char *);
> +extern int zpool_get_bootenv(zpool_handle_t *, char *, size_t, off_t);
> +
> +extern int libzfs_errno(libzfs_handle_t *);

Formatting is not correct for this function prototypes. Please take a
look a bit above...

>  #endif /* ! HAVE_LIBZFS_H */
>
>  libzfs_handle_t *grub_get_libzfs_handle (void);
> diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> index f3662c95b..b636ed1b8 100644
> --- a/util/grub-editenv.c
> +++ b/util/grub-editenv.c
> @@ -24,7 +24,12 @@
>  #include <grub/lib/envblk.h>
>  #include <grub/i18n.h>
>  #include <grub/emu/hostfile.h>
> +#include <grub/emu/hostdisk.h>
>  #include <grub/util/install.h>
> +#include <grub/util/libzfs.h>
> +#include <grub/emu/getroot.h>
> +#include <grub/fs.h>
> +#include <grub/crypto.h>
>
>  #include <stdio.h>
>  #include <unistd.h>
> @@ -36,7 +41,6 @@
>  #pragma GCC diagnostic error "-Wmissing-prototypes"
>  #pragma GCC diagnostic error "-Wmissing-declarations"
>
> -

OK but please mention in the commit message that you are doing some
cleanups too.

>  #include "progname.h"
>
>  #define DEFAULT_ENVBLK_PATH DEFAULT_DIRECTORY "/" GRUB_ENVBLK_DEFCFG
> @@ -120,6 +124,80 @@ block, use `rm %s'."),
>    NULL, help_filter, NULL
>  };
>
> +struct fs_envblk_spec {
> +  const char *fs_name;
> +  int (*fs_read) (void *, char *, size_t, off_t);
> +  int (*fs_write) (void *, const char *);
> +  void *(*fs_init) (grub_device_t);
> +  void (*fs_fini) (void *);
> +};
> +
> +struct fs_envblk {
> +  struct fs_envblk_spec *spec;
> +  grub_fs_t fs;
> +  void *data;
> +};
> +
> +typedef struct fs_envblk_spec fs_envblk_spec_t;
> +typedef struct fs_envblk fs_envblk_t;

Both typedefs should go immediately behind relevant struct
definitions. Good example is include/grub/menu.h:grub_menu_t.

> +fs_envblk_t *fs_envblk = NULL;
> +
> +#if defined(HAVE_LIBZFS) && defined(HAVE_LIBNVPAIR)
> +static void *
> +grub_zfs_init (grub_device_t dev)
> +{
> +  libzfs_handle_t *g_zfs = libzfs_init();
> +  int err;
> +  char *name;
> +  zpool_handle_t *zhp;
> +
> +  if (g_zfs == NULL)
> +    return NULL;
> +
> +  err = fs_envblk->fs->fs_label(dev, &name);
> +  if (err != GRUB_ERR_NONE) {
> +    libzfs_fini(g_zfs);
> +    return NULL;
> +  }
> +  zhp = zpool_open(g_zfs, name);
> +  if (zhp == NULL)
> +    {
> +      libzfs_fini(g_zfs);
> +      return NULL;
> +    }
> +  return zhp;
> +}
> +
> +static void
> +grub_zfs_fini (void *arg)
> +{
> +  zpool_handle_t *zhp = arg;
> +  libzfs_handle_t *g_zfs = zpool_get_handle(zhp);
> +  zpool_close(zhp);
> +  libzfs_fini(g_zfs);
> +}
> +
> +/* We need to convert ZFS's error returning pattern to the one we expect */
> +static int
> +grub_zfs_get_bootenv (void *arg, char *buf, size_t size, off_t offset)
> +{
> +  zpool_handle_t *zhp = arg;
> +  int error = zpool_get_bootenv (zhp, buf, size, offset);
> +  if (error != -1)
> +    return error;
> +  error = libzfs_errno(zpool_get_handle(zhp));
> +  return error;
> +}
> +#endif
> +
> +fs_envblk_spec_t fs_envblk_table[] = {
> +#if defined(HAVE_LIBZFS) && defined(HAVE_LIBNVPAIR)
> +  { "zfs", grub_zfs_get_bootenv, zpool_set_bootenv, grub_zfs_init,
> grub_zfs_fini},
> +#endif
> +  { NULL, NULL, NULL, NULL, NULL }
> +};
> +
>  static grub_envblk_t
>  open_envblk_file (const char *name)
>  {
> @@ -164,6 +242,51 @@ open_envblk_file (const char *name)
>    return envblk;
>  }
>
> +static grub_envblk_t
> +open_envblk (const char *name)
> +{
> +  char *buf = NULL;
> +  off_t off = 0;
> +  size_t size = 1024;

Could you use a constant here?

> +  grub_envblk_t envblk;
> +
> +  if (fs_envblk == NULL)
> +    return open_envblk_file(name);
> +
> +  while (1)
> +    {
> +      int res;

s/res/rc/

...and empty line here...

> +      buf = xrealloc(buf, size);
> +      res = fs_envblk->spec->fs_read(fs_envblk->data, buf + off, size, off);
> +      if (res < 0)
> +    {
> +      grub_util_error (_("cannot read envblock: %s"), strerror (errno));
> +      free(buf);
> +      return NULL;
> +    }
> +      if (res < size)
> +    {
> +      envblk = grub_envblk_open (buf, res + off);
> +      if (! envblk)
> +        grub_util_error ("%s", _("invalid environment block"));
> +      return envblk;
> +    }
> +      off += size;
> +      size *= 2;

What is happening here?

> +    }
> +}
> +
> +static void
> +close_envblk (grub_envblk_t envblk)
> +{
> +  grub_envblk_close (envblk);
> +  if (fs_envblk != NULL)
> +    {
> +      fs_envblk->spec->fs_fini (fs_envblk->data);
> +      fs_envblk->data = NULL;
> +    }
> +}

Yeah, here is good/proper close!

> +
>  static int
>  print_var (const char *varname, const char *value,
>             void *hook_data __attribute__ ((unused)))
> @@ -177,13 +300,13 @@ list_variables (const char *name)
>  {
>    grub_envblk_t envblk;
>
> -  envblk = open_envblk_file (name);
> +  envblk = open_envblk (name);
>    grub_envblk_iterate (envblk, NULL, print_var);
> -  grub_envblk_close (envblk);
> +  close_envblk (envblk);
>  }
>
>  static void
> -write_envblk (const char *name, grub_envblk_t envblk)
> +write_envblk_file (const char *name, grub_envblk_t envblk)
>  {
>    FILE *fp;
>
> @@ -202,12 +325,37 @@ write_envblk (const char *name, grub_envblk_t envblk)
>    fclose (fp);
>  }
>
> +static void
> +write_envblk (const char *name, grub_envblk_t envblk)
> +{
> +  if (fs_envblk == NULL)
> +    return write_envblk_file(name, envblk);
> +
> +  if (fs_envblk->spec->fs_write (fs_envblk->data, grub_envblk_buffer
> (envblk)) != 0)
> +    grub_util_error (_("cannot write to envblock: %s"), strerror (errno));
> +}
> +
> +static void
> +create_envblk (const char *name)
> +{
> +  char *buf;
> +  grub_envblk_t envblk;

Empty line here...

> +  if (fs_envblk == NULL)
> +    grub_util_create_envblk_file (name);
> +  buf = xmalloc (1024);
> +  grub_util_create_envblk_buffer(buf, 1024);
> +
> +  envblk = grub_envblk_open (buf, 1024);

Please use constants instead of plain numbers.

> +  write_envblk (name, envblk);
> +  close_envblk (envblk);
> +}
> +
>  static void
>  set_variables (const char *name, int argc, char *argv[])
>  {
>    grub_envblk_t envblk;
>
> -  envblk = open_envblk_file (name);
> +  envblk = open_envblk (name);
>    while (argc)
>      {
>        char *p;
> @@ -226,7 +374,7 @@ set_variables (const char *name, int argc, char *argv[])
>      }
>
>    write_envblk (name, envblk);
> -  grub_envblk_close (envblk);
> +  close_envblk (envblk);
>  }
>
>  static void
> @@ -234,7 +382,7 @@ unset_variables (const char *name, int argc, char *argv[])
>  {
>    grub_envblk_t envblk;
>
> -  envblk = open_envblk_file (name);
> +  envblk = open_envblk (name);
>    while (argc)
>      {
>        grub_envblk_delete (envblk, argv[0]);
> @@ -244,7 +392,117 @@ unset_variables (const char *name, int argc, char 
> *argv[])
>      }
>
>    write_envblk (name, envblk);
> -  grub_envblk_close (envblk);
> +  close_envblk (envblk);
> +}
> +
> +int have_abstraction = 0;
> +static void
> +probe_abstraction (grub_disk_t disk)
> +{
> +  if (disk->partition == NULL)
> +    grub_util_info ("no partition map found for %s", disk->name);
> +
> +  if (disk->dev->id == GRUB_DISK_DEVICE_DISKFILTER_ID ||
> +      disk->dev->id == GRUB_DISK_DEVICE_CRYPTODISK_ID)
> +    {
> +      have_abstraction = 1;

Could not this function return info about abstraction instead of setting
the global value?

> +    }

Curly braces are not needed here.

> + }
> +
> +static fs_envblk_t *
> +probe_fs_envblk (fs_envblk_spec_t *spec)
> +{
> +  char **grub_devices;
> +  char **curdev, **curdrive;
> +  size_t ndev = 0;
> +  char **grub_drives;
> +  grub_device_t grub_dev = NULL;
> +  grub_fs_t grub_fs;
> +  const char *fs_envblk_device;
> +
> +#ifdef __s390x__
> +  return NULL;
> +#endif

This is not nice. I think that you should use #else here too.

> +
> +  grub_util_biosdisk_init (DEFAULT_DEVICE_MAP);
> +  grub_init_all ();
> +  grub_gcry_init_all ();
> +
> +  grub_lvm_fini ();
> +  grub_mdraid09_fini ();
> +  grub_mdraid1x_fini ();
> +  grub_diskfilter_fini ();
> +  grub_diskfilter_init ();
> +  grub_mdraid09_init ();
> +  grub_mdraid1x_init ();
> +  grub_lvm_init ();
> +
> +  grub_devices = grub_guess_root_devices (DEFAULT_DIRECTORY);
> +
> +  if (!grub_devices || !grub_devices[0])
> +    grub_util_error (_("cannot find a device for %s (is /dev
> mounted?)"), DEFAULT_DIRECTORY);
> +
> +  fs_envblk_device = grub_devices[0];
> +
> +  for (curdev = grub_devices; *curdev; curdev++)
> +    {
> +      grub_util_pull_device (*curdev);
> +      ndev++;
> +    }
> +
> +  grub_drives = xmalloc (sizeof (grub_drives[0]) * (ndev + 1));
> +
> +  for (curdev = grub_devices, curdrive = grub_drives; *curdev; curdev++,
> +       curdrive++)
> +    {
> +      *curdrive = grub_util_get_grub_dev (*curdev);
> +      if (! *curdrive)
> +    grub_util_error (_("cannot find a GRUB drive for %s.  Check your
> device.map"),
> +             *curdev);
> +    }
> +  *curdrive = 0;

s/0/NULL/

> +  grub_dev = grub_device_open (grub_drives[0]);
> +
> +  grub_fs = grub_fs_probe (grub_dev);
> +
> +  if (grub_dev->disk)
> +    {
> +     probe_abstraction (grub_dev->disk);
> +    }

Please drop curly braces here.

> +  for (curdrive = grub_drives + 1; *curdrive; curdrive++)
> +    {
> +      grub_device_t dev = grub_device_open (*curdrive);
> +      if (!dev)
> +    continue;
> +      if (dev->disk)
> +    probe_abstraction (dev->disk);
> +      grub_device_close (dev);
> +    }
> +
> +  free (grub_drives);
> +  grub_gcry_fini_all ();
> +  grub_fini_all ();
> +  grub_util_biosdisk_fini ();
> +
> +  fs_envblk_spec_t *p;

Please move this to the beginning of the function.

Daniel