Hi Avnish, Sorry for the late reply. Please check my comments below:
On Wed, Sep 17, 2025 at 04:13:21PM +0530, Avnish Chouhan wrote: > On 2025-09-15 14:39, [email protected] wrote: > > Message: 3 > > Date: Mon, 15 Sep 2025 17:08:41 +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 2/9] util/grub-editenv: add fs_envblk open helper > > Message-ID: <[email protected]> > > > > 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 > > 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 > > 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 > > 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 > > variables are still stored in the file envblk and no redirection has > > been applied. > > > > Signed-off-by: Michael Chang <[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 2302a6acf..4e5dffa86 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); > > +}; > > + > > struct fs_envblk { > > struct fs_envblk_spec *spec; > > + struct fs_envblk_ops *ops; > > 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) > > +{ > > + grub_envblk_t *p_envblk = (grub_envblk_t *)hook_data; > > + 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); > > + 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) > > Hi Michael, > > (fp == NULL) would be better here! OK. I'll change it next version. > > > + grub_util_error (_("cannot open `%s': %s"), fs_envblk->dev, > > + strerror (errno)); > > + > > + if (fseek (fp, off, SEEK_SET) < 0) > > fclose (fp) seems missing here. It is not a mistake. The housekeeping cleanup is not required because the ensuing grub_util_error() aborts and terminates the program completely. > > > + grub_util_error (_("cannot seek `%s': %s"), fs_envblk->dev, > > + strerror (errno)); > > + > > + buf = xmalloc (sz); > > + if ((fread (buf, 1, sz, fp)) != sz) > > fclose (fp) seems missing as well as grub_free (buf). It is not required. Please check the comment above. > > > + 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) > > +{ > > + 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) > > fclose (fp) seems missing as well as grub_free (buf). It is not required. Please check the comment above. > > > + grub_util_error (_("cannot seek `%s': %s"), device, strerror > > (errno)); > > + > > + if (fwrite (buf, 1, size, fp) != size) > > fclose (fp) seems missing as well as grub_free (buf). It is not required. Please check the comment above. > > > + 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) > > + 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); > > + if (! grub_envblk_set (envblk, "env_block", val)) > > grub_free (val) missing here. It is not required. Please check the comment above. Thanks, Michael > > Thank you! > > Regards, > Avnish Chouhan > > > + grub_util_error ("%s", _("environment block too small")); > > + grub_envblk_iterate (envblk, &envblk_fs, read_envblk_fs); > > + free (val); > > + > > + return envblk_fs; > > +} > > + > > static grub_envblk_t > > open_envblk_file (const char *name) > > { > > @@ -392,6 +519,7 @@ probe_fs_envblk (fs_envblk_spec_t spec) > > fs_envblk = xmalloc (sizeof (fs_envblk_t)); > > fs_envblk->spec = p; > > fs_envblk->dev = devname; > > + fs_envblk->ops = &fs_envblk_ops; > > return fs_envblk; > > } > > } > > -- > > 2.51.0 _______________________________________________ Grub-devel mailing list [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
