On Tue, Sep 02, 2025 at 02:45:57PM +0800, Michael Chang via Grub-devel wrote: > 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
Where this value come from? Please explain how did you come up with it? What are pros and cons of it? Etc... > 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 The project name is GRUB not grub. Please use it correctly. Though probably I would use here grub-editenv instead... > 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 I can understand this restriction to some extent but I think it should be possible to lift it with --force argument. And of course Btrfs detection should be reliable... > 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]> > Reviewed-by: Neal Gompa <[email protected]> I consider patch splitting as significant change. So, IMO Neal's RB should be dropped from all patches... > --- > include/grub/fs.h | 2 + > util/grub-editenv.c | 156 +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 157 insertions(+), 1 deletion(-) > > diff --git a/include/grub/fs.h b/include/grub/fs.h > index df4c93b16..9c8206133 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) 256 * 1024... > #endif /* ! GRUB_FS_HEADER */ > diff --git a/util/grub-editenv.c b/util/grub-editenv.c > index db6f187cc..a1fa711cd 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,25 @@ block, use `rm %s'."), > NULL, help_filter, NULL > }; > > +struct fs_envblk_spec { > + const char *fs_name; > + int offset; > + int size; > +} fs_envblk_spec[] = { May I ask you to not conflate types and variables definitions? > + { "btrfs", ENV_BTRFS_OFFSET * 1024, GRUB_DISK_SECTOR_SIZE }, ... and drop multiplication from here... > + { NULL, 0, 0 } > +}; > + > +struct fs_envblk { > + struct fs_envblk_spec *spec; > + const char *dev; > +}; > + > +typedef struct fs_envblk_spec *fs_envblk_spec_t; > +typedef struct fs_envblk *fs_envblk_t; Please stick to GRUB coding style: struct fs_envblk { fs_envblk_spec_t spec; const char *dev; }; typedef struct fs_envblk *fs_envblk_t; Same for fs_envblk_spec_t please... > +fs_envblk_t fs_envblk = NULL; > + > static grub_envblk_t > open_envblk_file (const char *name) > { > @@ -253,6 +275,135 @@ unset_variables (const char *name, int argc, char > *argv[]) > grub_envblk_close (envblk); > } > > +static int Please use bool here. > +is_abstraction (grub_disk_t disk) > +{ > + if (disk->dev->id == GRUB_DISK_DEVICE_DISKFILTER_ID || > + disk->dev->id == GRUB_DISK_DEVICE_CRYPTODISK_ID) > + return 1; > + return 0; > +} > + > +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; > + int have_abstraction = 0; bool? > + 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 == 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]); > + > + for (curdev = grub_devices; *curdev; curdev++) > + { > + grub_util_pull_device (*curdev); > + ndev++; If you put ndev++ behind curdev++ then you can drop three lines of code... > + } > + > + grub_drives = xcalloc ((ndev + 1), sizeof (grub_drives[0])); > + > + 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; > + } > + > + if (grub_dev->disk) grub_dev->disk != NULL > + { > + have_abstraction |= is_abstraction (grub_dev->disk); Hmmm... Why not "=" instead of "|="? If yes then probably you can drop have_abstraction initialization at the beginning of the function. > + } You can drop curly braces here... > + for (curdrive = grub_drives + 1; *curdrive; curdrive++) s/*curdrive/*curdrive != NULL/ I can see similar minor issues in many places. Please fix it... > + { > + grub_device_t dev = grub_device_open (*curdrive); Please add an empty line here... > + if (dev == NULL) > + continue; > + if (dev->disk) > + have_abstraction |= is_abstraction (dev->disk); Again, s/|=/=/? Ahhh... Probably this should be if (is_abstraction (dev->disk) == true) have_abstraction = true; And why could not you move "if (dev->disk != NULL)" check into the is_abstraction() function? > + grub_device_close (dev); > + } > + > + cleanup: > + if (grub_devices != NULL) > + for (curdev = grub_devices; *curdev; curdev++) > + free (*curdev); > + free (grub_devices); > + free (grub_drives); > + grub_device_close (grub_dev); > + grub_gcry_fini_all (); > + grub_fini_all (); > + grub_util_biosdisk_fini (); > + > + if (grub_fs == NULL) > + { > + free (devname); > + return NULL; > + } > + > + for (p = spec; p->fs_name; p++) > + { > + if (strcmp (grub_fs->name, p->fs_name) == 0 && !have_abstraction) > + { > + fs_envblk = xmalloc (sizeof (fs_envblk_t)); > + fs_envblk->spec = p; > + fs_envblk->dev = devname; > + return fs_envblk; > + } > + } > + > + free (devname); > + return NULL; > +} > + > + > int > main (int argc, char *argv[]) > { > @@ -284,6 +435,9 @@ main (int argc, char *argv[]) > command = argv[curindex++]; > } > > + if (strcmp (filename, DEFAULT_ENVBLK_PATH) == 0) > + fs_envblk = probe_fs_envblk (fs_envblk_spec); I am not convinced... I think it should be possible to skip this check when user knows what he/she is doing... Daniel _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
