Re: [PATCH v2 2/9] util/grub-editenv: add fs_envblk open helper

[Avnish Chouhan <avnish@linux.ibm.com>]

On 2025-09-15 14:39, grub-devel-request@gnu.org wrote:
> Message: 3
> Date: Mon, 15 Sep 2025 17:08:41 +0800
> From: Michael Chang <mchang@suse.com>
> To: The development of GNU GRUB <grub-devel@gnu.org>
> Cc: Neal Gompa <ngompa13@gmail.com>,	Marta Lewandowska
> 	<mlewando@redhat.com>
> Subject: [PATCH v2 2/9] util/grub-editenv: add fs_envblk open helper
> Message-ID: <20250915090848.131937-3-mchang@suse.com>
> 
> 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 <mchang@suse.com>
> ---
>  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!

> +    grub_util_error (_("cannot open `%s': %s"), fs_envblk->dev,
> +			strerror (errno));
> +
> +  if (fseek (fp, off, SEEK_SET) < 0)

fclose (fp) seems missing here.

> +    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).

> +    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).

> +    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).

> +    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.

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
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel