Re: [PATCH v2 1/9] util/grub-editenv: add basic structures and probe call for external envblk

Re: [PATCH v2 1/9] util/grub-editenv: add basic structures and probe call for external envblk

[Avnish Chouhan]

On 2025-09-15 14:39, grub-devel-request@gnu.org wrote:
> 
> Message: 2
> Date: Mon, 15 Sep 2025 17:08:40 +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 1/9] util/grub-editenv: add basic structures and
> 	probe call for external envblk
> Message-ID: <20250915090848.131937-2-mchang@suse.com>
> 
> 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
> 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
> 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
> 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 <mchang@suse.com>
> ---
>  include/grub/fs.h   |   2 +
>  util/grub-editenv.c | 153 +++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 154 insertions(+), 1 deletion(-)
> 
> diff --git a/include/grub/fs.h b/include/grub/fs.h
> index df4c93b16..1be26dfba 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 * 1024)
> +
>  #endif /* ! GRUB_FS_HEADER */
> diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> index db6f187cc..2302a6acf 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,26 @@ block, use `rm %s'."),
>    NULL, help_filter, NULL
>  };
> 
> +struct fs_envblk_spec {

Hi Michael,

This '{' seems to be in a new line!

> +  const char *fs_name;
> +  off_t offset;
> +  size_t size;
> +};
> +typedef struct fs_envblk_spec *fs_envblk_spec_t;
> +
> +struct fs_envblk {

Same as above!

> +  struct fs_envblk_spec *spec;
> +  const char *dev;
> +};
> +typedef struct fs_envblk *fs_envblk_t;
> +
> +static struct fs_envblk_spec fs_envblk_spec[] = {
> +  { "btrfs", ENV_BTRFS_OFFSET, GRUB_DISK_SECTOR_SIZE },
> +  { NULL, 0, 0 }
> +};
> +
> +static fs_envblk_t fs_envblk = NULL;
> +
>  static grub_envblk_t
>  open_envblk_file (const char *name)
>  {
> @@ -253,6 +276,131 @@ unset_variables (const char *name, int argc, char 
> *argv[])
>    grub_envblk_close (envblk);
>  }
> 
> +static bool
> +is_abstraction (grub_device_t dev)
> +{
> +  if (dev == NULL || dev->disk == NULL)
> +    return false;
> +
> +  if (dev->disk->dev->id == GRUB_DISK_DEVICE_DISKFILTER_ID ||
> +      dev->disk->dev->id == GRUB_DISK_DEVICE_CRYPTODISK_ID)
> +    return true;

Adding an empty line would be better here!

> +  return false;
> +}
> +
> +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;
> +  bool have_abstraction = false;
> +
> +  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 ();

We aren't checking any failure in these initializes? Are we good without 
the checks here.. I'm not sure though!

> +
> +  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]);

Using grub_strdup would be better. We can check for NULL return in case 
of failure!

> +
> +  for (curdev = grub_devices; *curdev; curdev++, ndev++)
> +    grub_util_pull_device (*curdev);
> +
> +  grub_drives = xcalloc ((ndev + 1), sizeof (grub_drives[0]));

Same as above. grub_calloc...

> +
> +  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;
> +    }
> +
> +  have_abstraction = is_abstraction (grub_dev);
> +  for (curdrive = grub_drives + 1; *curdrive && have_abstraction ==
> false; curdrive++)
> +    {
> +      grub_device_t dev = grub_device_open (*curdrive);
> +      if (dev == NULL)
> +	continue;
> +      have_abstraction = is_abstraction (dev);
> +      grub_device_close (dev);
> +    }
> +
> + cleanup:
> +  if (grub_devices != NULL)
> +    for (curdev = grub_devices; *curdev; curdev++)
> +      free (*curdev);
> +  free (grub_devices);
> +  free (grub_drives);

grub_free() instead of free().

> +  grub_device_close (grub_dev);
> +  grub_gcry_fini_all ();
> +  grub_fini_all ();
> +  grub_util_biosdisk_fini ();
> +
> +  if (grub_fs == NULL)
> +    {
> +      free (devname);

grub_free() instead of free().

> +      return NULL;
> +    }
> +
> +  for (p = spec; p->fs_name; p++)
> +    {
> +      if (strcmp (grub_fs->name, p->fs_name) == 0 && 
> !have_abstraction)

Same as mentioned above. grub_strcmp()...

> +	{
> +	  fs_envblk = xmalloc (sizeof (fs_envblk_t));

Same as above. grub_malloc()...

> +	  fs_envblk->spec = p;
> +	  fs_envblk->dev = devname;
> +	  return fs_envblk;
> +	}
> +    }
> +
> +  free (devname);

grub_free() instead of free(). And adding a new line here would be good!

> +  return NULL;
> +}
> +
> +
>  int
>  main (int argc, char *argv[])
>  {
> @@ -284,6 +432,9 @@ main (int argc, char *argv[])
>        command  = argv[curindex++];
>      }
> 
> +  if (strcmp (filename, DEFAULT_ENVBLK_PATH) == 0)

Same as mentioned above. grub_strcmp()...

Thank you!

Regards,
Avnish Chouhan

> +    fs_envblk = probe_fs_envblk (fs_envblk_spec);
> +
>    if (strcmp (command, "create") == 0)
>      grub_util_create_envblk_file (filename);
>    else if (strcmp (command, "list") == 0)
> --
> 2.51.0

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 1/9] util/grub-editenv: add basic structures and probe call for external envblk

[Michael Chang via Grub-devel]

Hi Avnish,

Thanks for the review. Please see my comments below.

On Mon, Sep 15, 2025 at 06:37:05PM +0530, Avnish Chouhan wrote:
> On 2025-09-15 14:39, grub-devel-request@gnu.org wrote:
> > 
> > Message: 2
> > Date: Mon, 15 Sep 2025 17:08:40 +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 1/9] util/grub-editenv: add basic structures and
> > 	probe call for external envblk
> > Message-ID: <20250915090848.131937-2-mchang@suse.com>
> > 
> > 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
> > 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
> > 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
> > 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 <mchang@suse.com>
> > ---
> >  include/grub/fs.h   |   2 +
> >  util/grub-editenv.c | 153 +++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 154 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/grub/fs.h b/include/grub/fs.h
> > index df4c93b16..1be26dfba 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 * 1024)
> > +
> >  #endif /* ! GRUB_FS_HEADER */
> > diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> > index db6f187cc..2302a6acf 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,26 @@ block, use `rm %s'."),
> >    NULL, help_filter, NULL
> >  };
> > 
> > +struct fs_envblk_spec {
> 
> Hi Michael,
> 
> This '{' seems to be in a new line!
> 
> > +  const char *fs_name;
> > +  off_t offset;
> > +  size_t size;
> > +};
> > +typedef struct fs_envblk_spec *fs_envblk_spec_t;
> > +
> > +struct fs_envblk {
> 
> Same as above!

Are you suggesting adding a blank line between the struct and its
typedef? AFAIK the GNU style does not really require this. In the GRUB
project, and in other projects it includes, I see more often than not
the typedef written immediately after the struct without a blank line,
which would make it clearer that the typedef belongs to that struct.

> 
> > +  struct fs_envblk_spec *spec;
> > +  const char *dev;
> > +};
> > +typedef struct fs_envblk *fs_envblk_t;
> > +
> > +static struct fs_envblk_spec fs_envblk_spec[] = {
> > +  { "btrfs", ENV_BTRFS_OFFSET, GRUB_DISK_SECTOR_SIZE },
> > +  { NULL, 0, 0 }
> > +};
> > +
> > +static fs_envblk_t fs_envblk = NULL;
> > +
> >  static grub_envblk_t
> >  open_envblk_file (const char *name)
> >  {
> > @@ -253,6 +276,131 @@ unset_variables (const char *name, int argc, char
> > *argv[])
> >    grub_envblk_close (envblk);
> >  }
> > 
> > +static bool
> > +is_abstraction (grub_device_t dev)
> > +{
> > +  if (dev == NULL || dev->disk == NULL)
> > +    return false;
> > +
> > +  if (dev->disk->dev->id == GRUB_DISK_DEVICE_DISKFILTER_ID ||
> > +      dev->disk->dev->id == GRUB_DISK_DEVICE_CRYPTODISK_ID)
> > +    return true;
> 
> Adding an empty line would be better here!

OK, will do it next version.

> 
> > +  return false;
> > +}
> > +
> > +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;
> > +  bool have_abstraction = false;
> > +
> > +  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 ();
> 
> We aren't checking any failure in these initializes? Are we good without the
> checks here.. I'm not sure though!

These functions do not return a value. They are boilerplate
initialization code for GRUB modules and are often copied from other
utilities.

> 
> > +
> > +  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]);
> 
> Using grub_strdup would be better. We can check for NULL return in case of
> failure!

Well xstrdup() and other x* functions like xmalloc() and xcalloc()
already handle the NULL check, so we do not need to add repetitive NULL
checks and error handling in the code. This makes the code more concise
and less verbose IMHO.

> 
> > +
> > +  for (curdev = grub_devices; *curdev; curdev++, ndev++)
> > +    grub_util_pull_device (*curdev);
> > +
> > +  grub_drives = xcalloc ((ndev + 1), sizeof (grub_drives[0]));
> 
> Same as above. grub_calloc...

See also above ...

> 
> > +
> > +  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;
> > +    }
> > +
> > +  have_abstraction = is_abstraction (grub_dev);
> > +  for (curdrive = grub_drives + 1; *curdrive && have_abstraction ==
> > false; curdrive++)
> > +    {
> > +      grub_device_t dev = grub_device_open (*curdrive);
> > +      if (dev == NULL)
> > +	continue;
> > +      have_abstraction = is_abstraction (dev);
> > +      grub_device_close (dev);
> > +    }
> > +
> > + cleanup:
> > +  if (grub_devices != NULL)
> > +    for (curdev = grub_devices; *curdev; curdev++)
> > +      free (*curdev);
> > +  free (grub_devices);
> > +  free (grub_drives);
> 
> grub_free() instead of free().

No. grub_free() is just a wrapper for free() in this case, and since
this is a host utility entirely we do not need the additional detour. A
plain free() is sufficient here.

> 
> > +  grub_device_close (grub_dev);
> > +  grub_gcry_fini_all ();
> > +  grub_fini_all ();
> > +  grub_util_biosdisk_fini ();
> > +
> > +  if (grub_fs == NULL)
> > +    {
> > +      free (devname);
> 
> grub_free() instead of free().

See also above.

> 
> > +      return NULL;
> > +    }
> > +
> > +  for (p = spec; p->fs_name; p++)
> > +    {
> > +      if (strcmp (grub_fs->name, p->fs_name) == 0 && !have_abstraction)
> 
> Same as mentioned above. grub_strcmp()...

See also above.

> 
> > +	{
> > +	  fs_envblk = xmalloc (sizeof (fs_envblk_t));
> 
> Same as above. grub_malloc()...

See also above.

> 
> > +	  fs_envblk->spec = p;
> > +	  fs_envblk->dev = devname;
> > +	  return fs_envblk;
> > +	}
> > +    }
> > +
> > +  free (devname);
> 
> grub_free() instead of free().

See also above.

> And adding a new line here would be good!

OK. I'll add it in next version.

> 
> > +  return NULL;
> > +}
> > +
> > +
> >  int
> >  main (int argc, char *argv[])
> >  {
> > @@ -284,6 +432,9 @@ main (int argc, char *argv[])
> >        command  = argv[curindex++];
> >      }
> > 
> > +  if (strcmp (filename, DEFAULT_ENVBLK_PATH) == 0)
> 
> Same as mentioned above. grub_strcmp()...

See above, no need to take extra trip as we know all is in a utility
context solely.

> 
> Thank you!

I hope the clarification makes sense to you.

Thanks,
Michael

> 
> Regards,
> Avnish Chouhan
> 
> > +    fs_envblk = probe_fs_envblk (fs_envblk_spec);
> > +
> >    if (strcmp (command, "create") == 0)
> >      grub_util_create_envblk_file (filename);
> >    else if (strcmp (command, "list") == 0)
> > --
> > 2.51.0

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 1/9] util/grub-editenv: add basic structures and probe call for external envblk

[Avnish Chouhan]

On 2025-09-16 10:28, Michael Chang wrote:
> Hi Avnish,
> 
> Thanks for the review. Please see my comments below.
> 
> On Mon, Sep 15, 2025 at 06:37:05PM +0530, Avnish Chouhan wrote:
>> On 2025-09-15 14:39, grub-devel-request@gnu.org wrote:
>> >
>> > Message: 2
>> > Date: Mon, 15 Sep 2025 17:08:40 +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 1/9] util/grub-editenv: add basic structures and
>> > 	probe call for external envblk
>> > Message-ID: <20250915090848.131937-2-mchang@suse.com>
>> >
>> > 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
>> > 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
>> > 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
>> > 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 <mchang@suse.com>
> 
> Well xstrdup() and other x* functions like xmalloc() and xcalloc()
> already handle the NULL check, so we do not need to add repetitive NULL
> checks and error handling in the code. This makes the code more concise
> and less verbose IMHO.
> 
> 
> I hope the clarification makes sense to you.
> 
> Thanks,
> Michael
> 
>> 
>> Regards,
>> Avnish Chouhan

Hi Michael,
Thank you so much for your clarification!

In my opinion, using grub_*() would be better instead of x*(). My 
suggestion here was not about adding a NULL check, it was about "we can 
use grub_*() and add a NULL check instead of using x*()". Using grub_*() 
would be safe!

Reviewed-by: Avnish Chouhan <avnish@linux.ibm.com>

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Avnish Chouhan]

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

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

[Michael Chang via Grub-devel]

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, 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!

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

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

[Avnish Chouhan]

On 2025-10-02 11:14, Michael Chang wrote:
> 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, 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>

Reviewed-by: Avnish Chouhan <avnish@linux.ibm.com>

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 3/9] util/grub-editenv: add fs_envblk write helper

[Avnish Chouhan]

On 2025-09-15 14:39, grub-devel-request@gnu.org wrote:
  > Message: 4
> Date: Mon, 15 Sep 2025 17:08:42 +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 3/9] util/grub-editenv: add fs_envblk write helper
> Message-ID: <20250915090848.131937-4-mchang@suse.com>
> 
> This patch adds the function fs_envblk_write to update the reserved
> environment block on disk. The helper takes an in memory envblk buffer
> and writes it back to the device at the location defined by the
> fs_envblk specification. It performs size checks and uses file sync to
> ensure that the updated data is flushed.
> 
> The helper is also added into the fs_envblk ops table, together with 
> the
> open helper from the previous patch. With this change the basic input
> and output path for an external environment block is complete. The
> choice of which variables should be written externally will be handled
> by later patches.
> 
> Signed-off-by: Michael Chang <mchang@suse.com>
> ---
>  util/grub-editenv.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> index 4e5dffa86..26a81d2d0 100644
> --- a/util/grub-editenv.c
> +++ b/util/grub-editenv.c
> @@ -131,9 +131,11 @@ struct fs_envblk_spec {
>  typedef struct fs_envblk_spec *fs_envblk_spec_t;
> 
>  static grub_envblk_t fs_envblk_open (grub_envblk_t envblk);
> +static void fs_envblk_write (grub_envblk_t envblk);
> 
>  struct fs_envblk_ops {
>    grub_envblk_t (*open) (grub_envblk_t);
> +  void (*write) (grub_envblk_t);
>  };
> 
>  struct fs_envblk {
> @@ -144,7 +146,8 @@ struct fs_envblk {
>  typedef struct fs_envblk *fs_envblk_t;
> 
>  static struct fs_envblk_ops fs_envblk_ops = {
> -  .open = fs_envblk_open
> +  .open = fs_envblk_open,
> +  .write = fs_envblk_write
>  };
> 
>  static struct fs_envblk_spec fs_envblk_spec[] = {
> @@ -358,6 +361,39 @@ write_envblk (const char *name, grub_envblk_t 
> envblk)
>    fclose (fp);
>  }
> 
> +static void
> +fs_envblk_write (grub_envblk_t envblk)
> +{
> +  FILE *fp;
> +  const char *device;
> +  off_t offset;
> +  size_t size;
> +
> +  if (envblk == NULL)
> +    return;
> +
> +  device = fs_envblk->dev;
> +  offset = fs_envblk->spec->offset;
> +  size = fs_envblk->spec->size;
> +
> +  if (grub_envblk_size (envblk) > size)
> +    grub_util_error ("%s", _("environment block too small"));
> +
> +  fp = grub_util_fopen (device, "r+b");
> +
> +  if (! fp)

Hi Michael,

(fp == NUL) would be better here.

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

fclose (fp) missing here!

> +    grub_util_error (_("cannot seek `%s': %s"), device, strerror 
> (errno));
> +
> +  if (fwrite (grub_envblk_buffer (envblk), 1, grub_envblk_size
> (envblk), fp) != grub_envblk_size (envblk))

fclose (fp) missing here!

Thank you!

Regards,
Avnish Chouhan

> +    grub_util_error (_("cannot write to `%s': %s"), device, strerror 
> (errno));
> +
> +  grub_util_file_sync (fp);
> +  fclose (fp);
> +}
> +
>  static void
>  set_variables (const char *name, int argc, char *argv[])
>  {
> --
> 2.51.0

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 3/9] util/grub-editenv: add fs_envblk write helper

[Michael Chang via Grub-devel]

Hi Avnish,

Thanks a not for the review, please check my comments below.

On Wed, Sep 17, 2025 at 04:34:52PM +0530, Avnish Chouhan wrote:
> On 2025-09-15 14:39, grub-devel-request@gnu.org wrote:
>  > Message: 4
> > Date: Mon, 15 Sep 2025 17:08:42 +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 3/9] util/grub-editenv: add fs_envblk write helper
> > Message-ID: <20250915090848.131937-4-mchang@suse.com>
> > 
> > This patch adds the function fs_envblk_write to update the reserved
> > environment block on disk. The helper takes an in memory envblk buffer
> > and writes it back to the device at the location defined by the
> > fs_envblk specification. It performs size checks and uses file sync to
> > ensure that the updated data is flushed.
> > 
> > The helper is also added into the fs_envblk ops table, together with the
> > open helper from the previous patch. With this change the basic input
> > and output path for an external environment block is complete. The
> > choice of which variables should be written externally will be handled
> > by later patches.
> > 
> > Signed-off-by: Michael Chang <mchang@suse.com>
> > ---
> >  util/grub-editenv.c | 38 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 37 insertions(+), 1 deletion(-)
> > 
> > diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> > index 4e5dffa86..26a81d2d0 100644
> > --- a/util/grub-editenv.c
> > +++ b/util/grub-editenv.c
> > @@ -131,9 +131,11 @@ struct fs_envblk_spec {
> >  typedef struct fs_envblk_spec *fs_envblk_spec_t;
> > 
> >  static grub_envblk_t fs_envblk_open (grub_envblk_t envblk);
> > +static void fs_envblk_write (grub_envblk_t envblk);
> > 
> >  struct fs_envblk_ops {
> >    grub_envblk_t (*open) (grub_envblk_t);
> > +  void (*write) (grub_envblk_t);
> >  };
> > 
> >  struct fs_envblk {
> > @@ -144,7 +146,8 @@ struct fs_envblk {
> >  typedef struct fs_envblk *fs_envblk_t;
> > 
> >  static struct fs_envblk_ops fs_envblk_ops = {
> > -  .open = fs_envblk_open
> > +  .open = fs_envblk_open,
> > +  .write = fs_envblk_write
> >  };
> > 
> >  static struct fs_envblk_spec fs_envblk_spec[] = {
> > @@ -358,6 +361,39 @@ write_envblk (const char *name, grub_envblk_t
> > envblk)
> >    fclose (fp);
> >  }
> > 
> > +static void
> > +fs_envblk_write (grub_envblk_t envblk)
> > +{
> > +  FILE *fp;
> > +  const char *device;
> > +  off_t offset;
> > +  size_t size;
> > +
> > +  if (envblk == NULL)
> > +    return;
> > +
> > +  device = fs_envblk->dev;
> > +  offset = fs_envblk->spec->offset;
> > +  size = fs_envblk->spec->size;
> > +
> > +  if (grub_envblk_size (envblk) > size)
> > +    grub_util_error ("%s", _("environment block too small"));
> > +
> > +  fp = grub_util_fopen (device, "r+b");
> > +
> > +  if (! fp)
> 
> Hi Michael,
> 
> (fp == NUL) would be better here.

OK. Will be fixed in the next version.
> 
> > +    grub_util_error (_("cannot open `%s': %s"), device, strerror
> > (errno));
> > +
> > +  if (fseek (fp, offset, SEEK_SET) < 0)
> 
> fclose (fp) missing here!

IMHO it is not required as grub_util_error will terminate program with
the error reported.
> 
> > +    grub_util_error (_("cannot seek `%s': %s"), device, strerror
> > (errno));
> > +
> > +  if (fwrite (grub_envblk_buffer (envblk), 1, grub_envblk_size
> > (envblk), fp) != grub_envblk_size (envblk))
> 
> fclose (fp) missing here!

Same as above.

Thanks,
Michael

> 
> Thank you!
> 
> Regards,
> Avnish Chouhan
> 
> > +    grub_util_error (_("cannot write to `%s': %s"), device, strerror
> > (errno));
> > +
> > +  grub_util_file_sync (fp);
> > +  fclose (fp);
> > +}
> > +
> >  static void
> >  set_variables (const char *name, int argc, char *argv[])
> >  {
> > --
> > 2.51.0

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 3/9] util/grub-editenv: add fs_envblk write helper

[Avnish Chouhan]

On 2025-10-02 11:23, Michael Chang wrote:
> Hi Avnish,
> 
> Thanks a not for the review, please check my comments below.
> 
> On Wed, Sep 17, 2025 at 04:34:52PM +0530, Avnish Chouhan wrote:
>> On 2025-09-15 14:39, grub-devel-request@gnu.org wrote:
>>  > Message: 4
>> > Date: Mon, 15 Sep 2025 17:08:42 +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 3/9] util/grub-editenv: add fs_envblk write helper
>> > Message-ID: <20250915090848.131937-4-mchang@suse.com>
>> >
>> > This patch adds the function fs_envblk_write to update the reserved
>> > environment block on disk. The helper takes an in memory envblk buffer
>> > and writes it back to the device at the location defined by the
>> > fs_envblk specification. It performs size checks and uses file sync to
>> > ensure that the updated data is flushed.
>> >
>> > The helper is also added into the fs_envblk ops table, together with the
>> > open helper from the previous patch. With this change the basic input
>> > and output path for an external environment block is complete. The
>> > choice of which variables should be written externally will be handled
>> > by later patches.
>> >
>> > Signed-off-by: Michael Chang <mchang@suse.com>

Reviewed-by: Avnish Chouhan <avnish@linux.ibm.com>

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 0/9] Add support for external environment block on Btrfs

[Michael Chang via Grub-devel]

This patch series adds support for storing the GRUB environment block in
a reserved area of the Btrfs header. On copy on write filesystems such
as Btrfs, the normal file based envblk cannot be updated safely at
runtime because block addresses are not stable. The reserved area
provides a fixed location that GRUB can write directly, allowing
commands such as grub-reboot and savedefault to work on Btrfs volumes.

The series proceeds in small chunks to keep each change buildable and
easier to review. The first patches add new data structures and helpers
for creating, opening, and writing an environment block in the reserved
area. Later patches update set_variables, unset_variables, and
list_variables so they can use the external block when it is present. An
entry is added to the Btrfs header to reserve space at 256 KiB for the
environment block. Finally, grub.cfg is modified so that load_env and
save_env use the external block automatically when env_block is defined.

v2:
- Define ENV_BTRFS_OFFSET as 256*1024
- Do not conflate type and variable definitions
- Align typedef with struct declaration to follow coding style
- Use bool as the return type of is_abstraction()
- Add "if (dev->disk != NULL)" check in is_abstraction()
- Refine the loop logic in is_abstraction() tests
- Remove extra indentation and redundant lines in read_envblk_fs
- Use off_t and size_t for offset and size variables, fix similar cases
  throughout
- Use explicit check "(fp == NULL)" instead of "(! fp)", fix similar
  cases throughout
- Use bool for the "found" field in var_lookup_ctx
- Add documentation describing the Btrfs environment block and special
  environment block variables

Michael Chang (9):
  util/grub-editenv: add basic structures and probe call for external
    envblk
  util/grub-editenv: add fs_envblk open helper
  util/grub-editenv: add fs_envblk write helper
  util/grub-editenv: wire set_variables to optional fs_envblk
  util/grub-editenv: wire unset_variables to optional fs_envblk
  util/grub-editenv: wire list_variables to optional fs_envblk
  btrfs: add environment block to reserved header area
  00_header.in: wire grub.cfg to use env_block when present
  docs: add Btrfs env block and special env vars

 docs/grub.texi           |  60 ++++++
 grub-core/fs/btrfs.c     |   3 +-
 include/grub/fs.h        |   2 +
 util/grub-editenv.c      | 393 ++++++++++++++++++++++++++++++++++++++-
 util/grub.d/00_header.in |  26 ++-
 5 files changed, 477 insertions(+), 7 deletions(-)

-- 
2.51.0


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Michael Chang via Grub-devel]

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