On Thu, Oct 02, 2025 at 05:33:51PM +0200, Daniel Kiper wrote: > On Thu, Oct 02, 2025 at 02:46:15PM +0800, Michael Chang via Grub-devel wrote: > > This patch adds the logic to locate and open an environment block that > > is stored in a reserved area on the device. It introduces the function > > fs_envblk_open together with helper routines to read the block pointed > > s/fs_envblk_open/fs_envblk_open()/
OK, will fix next version. > > > to by the env_block variable, and to create the block on disk when it > > does not yet exist. When a block is created, the code records its > > s/yet exist/exist yet/ OK, will fix next version. > > > location inside the file based envblk by setting env_block in block list > > syntax of offset plus size in sectors. > > > > The env_block variable acts as a link from the file envblk to the raw > > disk region so that later runs of grub editenv can follow it and access > > s/grub editenv/grub-editenv/ OK, will fix next version. > > > the external block. The helper is exposed through a small ops table > > attached to fs_envblk so that later patches can call > > fs_envblk->ops->open without touching core code again. At this stage > > fs_envblk->ops->open() OK, will fix next version. > > > variables are still stored in the file envblk and no redirection has > > been applied. > > > > Signed-off-by: Michael Chang <[email protected]> > > Reviewed-by: Neal Gompa <[email protected]> > > --- > > util/grub-editenv.c | 128 ++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 128 insertions(+) > > > > diff --git a/util/grub-editenv.c b/util/grub-editenv.c > > index 1f055ca43..829384392 100644 > > --- a/util/grub-editenv.c > > +++ b/util/grub-editenv.c > > @@ -130,12 +130,23 @@ struct fs_envblk_spec { > > }; > > typedef struct fs_envblk_spec *fs_envblk_spec_t; > > > > +static grub_envblk_t fs_envblk_open (grub_envblk_t envblk); > > + > > +struct fs_envblk_ops { > > + grub_envblk_t (*open) (grub_envblk_t); > > +}; > > typedef struct fs_envblk_ops struct fs_envblk_ops_t; OK, will fix next version. > > > struct fs_envblk { > > struct fs_envblk_spec *spec; > > + struct fs_envblk_ops *ops; > > fs_envblk_ops_t, etc... OK, will fix next version. > > > const char *dev; > > }; > > typedef struct fs_envblk *fs_envblk_t; > > > > +static struct fs_envblk_ops fs_envblk_ops = { > > + .open = fs_envblk_open > > +}; > > + > > static struct fs_envblk_spec fs_envblk_spec[] = { > > { "btrfs", ENV_BTRFS_OFFSET, GRUB_DISK_SECTOR_SIZE }, > > { NULL, 0, 0 } > > @@ -143,6 +154,122 @@ static struct fs_envblk_spec fs_envblk_spec[] = { > > > > static fs_envblk_t fs_envblk = NULL; > > > > +static int > > +read_envblk_fs (const char *varname, const char *value, void *hook_data) > > s/read_envblk_fs/read_env_block_var/ The suggested function name does look better, will fix next version. > > > +{ > > + grub_envblk_t *p_envblk = (grub_envblk_t *)hook_data; > > Missing space before hook_data... OK, will fix next version. > > > + off_t off; > > + size_t sz; > > + char *p, *buf; > > + FILE *fp; > > + > > + if (p_envblk == NULL || fs_envblk == NULL) > > + return 1; > > + > > + if (strcmp (varname, "env_block") != 0) > > + return 0; > > + > > + off = strtol (value, &p, 10); > > + if (*p == '+') > > + sz = strtol (p+1, &p, 10); > > s/p+1/p + 1/ OK, will fix next version. > > > + else > > + return 0; > > + > > + if (*p != '\0' || sz == 0) > > + return 0; > > + > > + off <<= GRUB_DISK_SECTOR_BITS; > > + sz <<= GRUB_DISK_SECTOR_BITS; > > + > > + fp = grub_util_fopen (fs_envblk->dev, "rb"); > > + if (fp == NULL) > > + grub_util_error (_("cannot open `%s': %s"), fs_envblk->dev, > > + strerror (errno)); > > You do not need wrap lines in cases like that one. I am OK with lines > (a bit) longer than 80 characters... OK. > > > + > > + if (fseek (fp, off, SEEK_SET) < 0) > > + grub_util_error (_("cannot seek `%s': %s"), fs_envblk->dev, > > + strerror (errno)); > > Ditto and below... Ok. > > > + buf = xmalloc (sz); > > + if ((fread (buf, 1, sz, fp)) != sz) > > + grub_util_error (_("cannot read `%s': %s"), fs_envblk->dev, > > + strerror (errno)); > > + > > + fclose (fp); > > + > > + *p_envblk = grub_envblk_open (buf, sz); > > + > > + return 1; > > +} > > + > > +static void > > +create_envblk_fs (void) > > s/create_envblk_fs/create_env_on_block/ Looks better, will fix the function name to suggested one. > > > +{ > > + FILE *fp; > > + char *buf; > > + const char *device; > > + off_t offset; > > + size_t size; > > + > > + if (fs_envblk == NULL) > > + return; > > + > > + device = fs_envblk->dev; > > + offset = fs_envblk->spec->offset; > > + size = fs_envblk->spec->size; > > + > > + fp = grub_util_fopen (device, "r+b"); > > + if (fp == NULL) > > + grub_util_error (_("cannot open `%s': %s"), device, strerror (errno)); > > + > > + buf = xmalloc (size); > > + memcpy (buf, GRUB_ENVBLK_SIGNATURE, sizeof (GRUB_ENVBLK_SIGNATURE) - 1); > > + memset (buf + sizeof (GRUB_ENVBLK_SIGNATURE) - 1, '#', size - sizeof > > (GRUB_ENVBLK_SIGNATURE) + 1); > > + > > + if (fseek (fp, offset, SEEK_SET) < 0) > > + grub_util_error (_("cannot seek `%s': %s"), device, strerror (errno)); > > + > > + if (fwrite (buf, 1, size, fp) != size) > > + grub_util_error (_("cannot write to `%s': %s"), device, strerror > > (errno)); > > + > > + grub_util_file_sync (fp); > > + free (buf); > > + fclose (fp); > > +} > > + > > +static grub_envblk_t > > +fs_envblk_open (grub_envblk_t envblk) > > +{ > > + grub_envblk_t envblk_fs = NULL; > > + char *val; > > + off_t offset; > > + size_t size; > > + > > + if (envblk == NULL) > > + return NULL; > > + > > + offset = fs_envblk->spec->offset; > > + size = fs_envblk->spec->size; > > + > > + grub_envblk_iterate (envblk, &envblk_fs, read_envblk_fs); > > + > > + if (envblk_fs && grub_envblk_size (envblk_fs) == size) > > envblk_fs != NULL Ok. > > > + return envblk_fs; > > + > > + create_envblk_fs (); > > + > > + offset = offset >> GRUB_DISK_SECTOR_BITS; > > + size = (size + GRUB_DISK_SECTOR_SIZE - 1) >> GRUB_DISK_SECTOR_BITS; > > + > > + val = xasprintf ("%ld+%lu", offset, size); > > val = xasprintf ("%lld+%zu", (long long) offset, size); Ok, will fix the string output so it is portable and safe. > > > + if (! grub_envblk_set (envblk, "env_block", val)) > > + grub_util_error ("%s", _("environment block too small")); > > + grub_envblk_iterate (envblk, &envblk_fs, read_envblk_fs); > > + free (val); > > + > > + return envblk_fs; > > +} Thanks, Michael > > Daniel _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
