On Fri, Sep 05, 2025 at 09:39:56PM +0200, Daniel Kiper wrote: > 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...
It was decided long time ago that the exact reasoning is hard to trace now. IIRC the value was chosen so that it is not placed too close to the Btrfs superblock and also leaves some distance from the beginning of the data blocks. > > > 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... I'll correct it to grub-editenv next version. > > > 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... The scope of this series is to make that grub-reboot and savedefault work when /boot is on Btrfs and the boot scripts are generated by grub-mkconfig, in other words the default configuration. Other scenarios have not been tested, and it is unclear whether this change would be appropriate or desirable in those cases. Note that there is only one external environment block stored in the Btrfs header, while a Btrfs filesystem may have multiple subvolumes and snapshots, each with its own private grubenv file for settings. It is not always desirable to enable the external block by default as that may overlay settings, so the decision has been relatively conservative. > > > 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... OK. I will drop the Reviewed-by next version. > > > --- > > 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... OK. > > > #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? OK. > > > + { "btrfs", ENV_BTRFS_OFFSET * 1024, GRUB_DISK_SECTOR_SIZE }, > > ... and drop multiplication from here... OK. > > > + { 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... OK. > > > +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. OK. > > > +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? OK. > > > + 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... Yes. > > > + } > > + > > + 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 OK. > > > + { > > + 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... OK. > > > + { > > + grub_device_t dev = grub_device_open (*curdrive); > > Please add an empty line here... OK. > > > + 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; OK. I'll avoid "|=" to make the expression more easy to understand. > > And why could not you move "if (dev->disk != NULL)" check into the > is_abstraction() function? Yes, that makes sense to me. I'll move the check to the function as you said. > > > + 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... As above, I would like to keep this in the initial support and enable it per the request, as there is a clear usecase. > > Daniel _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
