Hi Avnish, Thanks for the review. Please see my comments below.
On Mon, Sep 15, 2025 at 06:37:05PM +0530, Avnish Chouhan wrote: > On 2025-09-15 14:39, [email protected] wrote: > > > > Message: 2 > > Date: Mon, 15 Sep 2025 17:08:40 +0800 > > From: Michael Chang <[email protected]> > > To: The development of GNU GRUB <[email protected]> > > Cc: Neal Gompa <[email protected]>, Marta Lewandowska > > <[email protected]> > > Subject: [PATCH v2 1/9] util/grub-editenv: add basic structures and > > probe call for external envblk > > Message-ID: <[email protected]> > > > > This patch prepares for using an environment block stored in a reserved > > area of the filesystem. It adds a constant ENV_BTRFS_OFFSET at 256 KiB > > in the Btrfs header. It also introduces the fs_envblk_spec and fs_envblk > > structures together with the probe_fs_envblk function to identify the > > root filesystem and to avoid configurations that involve diskfilter or > > cryptodisk. > > > > The probe is only invoked when grub-editenv is working on the default > > environment file path. This restriction ensures that probing and > > possible raw device access are not triggered for arbitrary user supplied > > paths, but only for the standard grubenv file. In that case the code > > checks if the filename equals DEFAULT_ENVBLK_PATH and then calls > > probe_fs_envblk with fs_envblk_spec. The result is stored in the global > > fs_envblk handle. At this stage the external environment block is only > > detected and recorded, and the behavior of grub editenv is unchanged. > > > > Signed-off-by: Michael Chang <[email protected]> > > --- > > include/grub/fs.h | 2 + > > util/grub-editenv.c | 153 +++++++++++++++++++++++++++++++++++++++++++- > > 2 files changed, 154 insertions(+), 1 deletion(-) > > > > diff --git a/include/grub/fs.h b/include/grub/fs.h > > index df4c93b16..1be26dfba 100644 > > --- a/include/grub/fs.h > > +++ b/include/grub/fs.h > > @@ -132,4 +132,6 @@ grub_fs_unregister (grub_fs_t fs) > > > > grub_fs_t EXPORT_FUNC(grub_fs_probe) (grub_device_t device); > > > > +#define ENV_BTRFS_OFFSET (256 * 1024) > > + > > #endif /* ! GRUB_FS_HEADER */ > > diff --git a/util/grub-editenv.c b/util/grub-editenv.c > > index db6f187cc..2302a6acf 100644 > > --- a/util/grub-editenv.c > > +++ b/util/grub-editenv.c > > @@ -23,8 +23,11 @@ > > #include <grub/util/misc.h> > > #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/emu/getroot.h> > > +#include <grub/fs.h> > > +#include <grub/crypto.h> > > > > #include <stdio.h> > > #include <unistd.h> > > @@ -120,6 +123,26 @@ block, use `rm %s'."), > > NULL, help_filter, NULL > > }; > > > > +struct fs_envblk_spec { > > Hi Michael, > > This '{' seems to be in a new line! > > > + const char *fs_name; > > + off_t offset; > > + size_t size; > > +}; > > +typedef struct fs_envblk_spec *fs_envblk_spec_t; > > + > > +struct fs_envblk { > > Same as above! Are you suggesting adding a blank line between the struct and its typedef? AFAIK the GNU style does not really require this. In the GRUB project, and in other projects it includes, I see more often than not the typedef written immediately after the struct without a blank line, which would make it clearer that the typedef belongs to that struct. > > > + struct fs_envblk_spec *spec; > > + const char *dev; > > +}; > > +typedef struct fs_envblk *fs_envblk_t; > > + > > +static struct fs_envblk_spec fs_envblk_spec[] = { > > + { "btrfs", ENV_BTRFS_OFFSET, GRUB_DISK_SECTOR_SIZE }, > > + { NULL, 0, 0 } > > +}; > > + > > +static fs_envblk_t fs_envblk = NULL; > > + > > static grub_envblk_t > > open_envblk_file (const char *name) > > { > > @@ -253,6 +276,131 @@ unset_variables (const char *name, int argc, char > > *argv[]) > > grub_envblk_close (envblk); > > } > > > > +static bool > > +is_abstraction (grub_device_t dev) > > +{ > > + if (dev == NULL || dev->disk == NULL) > > + return false; > > + > > + if (dev->disk->dev->id == GRUB_DISK_DEVICE_DISKFILTER_ID || > > + dev->disk->dev->id == GRUB_DISK_DEVICE_CRYPTODISK_ID) > > + return true; > > Adding an empty line would be better here! OK, will do it next version. > > > + return false; > > +} > > + > > +static fs_envblk_t > > +probe_fs_envblk (fs_envblk_spec_t spec) > > +{ > > + char **grub_devices = NULL; > > + char **curdev, **curdrive; > > + size_t ndev = 0; > > + char **grub_drives = NULL; > > + grub_device_t grub_dev = NULL; > > + grub_fs_t grub_fs = NULL; > > + char *devname = NULL; > > + fs_envblk_spec_t p; > > + bool have_abstraction = false; > > + > > + 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 (); > > We aren't checking any failure in these initializes? Are we good without the > checks here.. I'm not sure though! These functions do not return a value. They are boilerplate initialization code for GRUB modules and are often copied from other utilities. > > > + > > + grub_devices = grub_guess_root_devices (DEFAULT_DIRECTORY); > > + > > + if (grub_devices == NULL || grub_devices[0] == NULL) > > + { > > + grub_util_warn (_("cannot find a device for %s (is /dev > > mounted?)"), DEFAULT_DIRECTORY); > > + goto cleanup; > > + } > > + > > + devname = xstrdup (grub_devices[0]); > > Using grub_strdup would be better. We can check for NULL return in case of > failure! Well xstrdup() and other x* functions like xmalloc() and xcalloc() already handle the NULL check, so we do not need to add repetitive NULL checks and error handling in the code. This makes the code more concise and less verbose IMHO. > > > + > > + for (curdev = grub_devices; *curdev; curdev++, ndev++) > > + grub_util_pull_device (*curdev); > > + > > + grub_drives = xcalloc ((ndev + 1), sizeof (grub_drives[0])); > > Same as above. grub_calloc... See also above ... > > > + > > + for (curdev = grub_devices, curdrive = grub_drives; *curdev; > > curdev++, > > + curdrive++) > > + { > > + *curdrive = grub_util_get_grub_dev (*curdev); > > + if (*curdrive == NULL) > > + { > > + grub_util_warn (_("cannot find a GRUB drive for %s. Check your > > device.map"), > > + *curdev); > > + goto cleanup; > > + } > > + } > > + *curdrive = 0; > > + > > + grub_dev = grub_device_open (grub_drives[0]); > > + if (grub_dev == NULL) > > + { > > + grub_util_warn (_("cannot open device %s: %s"), grub_drives[0], > > grub_errmsg); > > + grub_errno = GRUB_ERR_NONE; > > + goto cleanup; > > + } > > + > > + grub_fs = grub_fs_probe (grub_dev); > > + if (grub_fs == NULL) > > + { > > + grub_util_warn (_("cannot probe fs for %s: %s"), > > grub_drives[0], grub_errmsg); > > + grub_errno = GRUB_ERR_NONE; > > + goto cleanup; > > + } > > + > > + have_abstraction = is_abstraction (grub_dev); > > + for (curdrive = grub_drives + 1; *curdrive && have_abstraction == > > false; curdrive++) > > + { > > + grub_device_t dev = grub_device_open (*curdrive); > > + if (dev == NULL) > > + continue; > > + have_abstraction = is_abstraction (dev); > > + grub_device_close (dev); > > + } > > + > > + cleanup: > > + if (grub_devices != NULL) > > + for (curdev = grub_devices; *curdev; curdev++) > > + free (*curdev); > > + free (grub_devices); > > + free (grub_drives); > > grub_free() instead of free(). No. grub_free() is just a wrapper for free() in this case, and since this is a host utility entirely we do not need the additional detour. A plain free() is sufficient here. > > > + grub_device_close (grub_dev); > > + grub_gcry_fini_all (); > > + grub_fini_all (); > > + grub_util_biosdisk_fini (); > > + > > + if (grub_fs == NULL) > > + { > > + free (devname); > > grub_free() instead of free(). See also above. > > > + return NULL; > > + } > > + > > + for (p = spec; p->fs_name; p++) > > + { > > + if (strcmp (grub_fs->name, p->fs_name) == 0 && !have_abstraction) > > Same as mentioned above. grub_strcmp()... See also above. > > > + { > > + fs_envblk = xmalloc (sizeof (fs_envblk_t)); > > Same as above. grub_malloc()... See also above. > > > + fs_envblk->spec = p; > > + fs_envblk->dev = devname; > > + return fs_envblk; > > + } > > + } > > + > > + free (devname); > > grub_free() instead of free(). See also above. > And adding a new line here would be good! OK. I'll add it in next version. > > > + return NULL; > > +} > > + > > + > > int > > main (int argc, char *argv[]) > > { > > @@ -284,6 +432,9 @@ main (int argc, char *argv[]) > > command = argv[curindex++]; > > } > > > > + if (strcmp (filename, DEFAULT_ENVBLK_PATH) == 0) > > Same as mentioned above. grub_strcmp()... See above, no need to take extra trip as we know all is in a utility context solely. > > Thank you! I hope the clarification makes sense to you. Thanks, Michael > > Regards, > Avnish Chouhan > > > + fs_envblk = probe_fs_envblk (fs_envblk_spec); > > + > > if (strcmp (command, "create") == 0) > > grub_util_create_envblk_file (filename); > > else if (strcmp (command, "list") == 0) > > -- > > 2.51.0 _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
