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 <p...@delphix.com> > AuthorDate: Mon Feb 24 14:29:47 2020 -0800 > Commit: Paul Dagnelie <p...@delphix.com> > 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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel