Re: [PATCH v4 07/12] util/grub-editenv: wire set_variables to optional fs_envblk

Re: [PATCH v4 07/12] util/grub-editenv: wire set_variables to optional fs_envblk

[Avnish Chouhan]

On 2025-10-09 12:50, grub-devel-request@gnu.org wrote:

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

> Message: 3
> Date: Thu,  9 Oct 2025 15:18: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 v4 07/12] util/grub-editenv: wire set_variables to
> 	optional fs_envblk
> Message-ID: <20251009071845.318828-8-mchang@suse.com>
> 
> This patch changes set_variables() so that it can use an external
> environment block when one is present. The variable next_entry is
> written into the external block, env_block is treated as read only, and
> all other variables are written into the normal file based envblk.
> 
> A cleanup step is added to handle cases where GRUB at runtime writes
> variables into the external block because file based updates are not
> safe on a copy on write filesystem such as Btrfs. For example, the
> savedefault command can update saved_entry, and on Btrfs GRUB will 
> place
> that update in the external block instead of the file envblk. If an
> older copy remains in the external block, it would override the newer
> value from the file envblk when GRUB first loads the file and then
> applies the external block on top of it. To avoid this, whenever a
> variable is updated in the file envblk, any same named key in the
> external block is deleted.
> 
> Signed-off-by: Michael Chang <mchang@suse.com>
> Reviewed-by: Neal Gompa <ngompa13@gmail.com>
> ---
>  util/grub-editenv.c | 58 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> index 42a0c63f8..beb2dd4fc 100644
> --- a/util/grub-editenv.c
> +++ b/util/grub-editenv.c
> @@ -402,12 +402,35 @@ fs_envblk_write (grub_envblk_t envblk)
>    fclose (fp);
>  }
> 
> +struct var_lookup_ctx {
> +  const char *varname;
> +  bool found;
> +};
> +typedef struct var_lookup_ctx var_lookup_ctx_t;
> +
> +static int
> +var_lookup_iter (const char *varname, const char *value __attribute__
> ((unused)), void *hook_data)
> +{
> +  var_lookup_ctx_t *ctx = (var_lookup_ctx_t *) hook_data;
> +
> +  if (grub_strcmp (ctx->varname, varname) == 0)
> +    {
> +      ctx->found = true;
> +      return 1;
> +    }
> +  return 0;
> +}
> +
>  static void
>  set_variables (const char *name, int argc, char *argv[])
>  {
>    grub_envblk_t envblk;
> +  grub_envblk_t envblk_on_block = NULL;
> 
>    envblk = open_envblk_file (name);
> +  if (fs_envblk != NULL)
> +    envblk_on_block = fs_envblk->ops->open (envblk);
> +
>    while (argc)
>      {
>        char *p;
> @@ -418,15 +441,46 @@ set_variables (const char *name, int argc, char 
> *argv[])
> 
>        *(p++) = 0;
> 
> -      if (! grub_envblk_set (envblk, argv[0], p))
> -        grub_util_error ("%s", _("environment block too small"));
> +      if ((strcmp (argv[0], "next_entry") == 0) && envblk_on_block != 
> NULL)
> +	{
> +	  if (grub_envblk_set (envblk_on_block, argv[0], p) == 0)
> +	    grub_util_error ("%s", _("environment block too small"));
> +	  goto next;
> +	}
> +
> +      if (strcmp (argv[0], "env_block") == 0)
> +	{
> +	  grub_util_warn (_("can't set env_block as it's read-only"));
> +	  goto next;
> +	}
> +
> +      if (grub_envblk_set (envblk, argv[0], p) == 0)
> +	grub_util_error ("%s", _("environment block too small"));
> 
> +      if (envblk_on_block != NULL)
> +	{
> +	  var_lookup_ctx_t ctx = {
> +	    .varname = argv[0],
> +	    .found = false
> +	  };
> +
> +	  grub_envblk_iterate (envblk_on_block, &ctx, var_lookup_iter);
> +	  if (ctx.found == true)
> +	    grub_envblk_delete (envblk_on_block, argv[0]);
> +	}
> + next:
>        argc--;
>        argv++;
>      }
> 
>    write_envblk (name, envblk);
>    grub_envblk_close (envblk);
> +
> +  if (envblk_on_block != NULL)
> +    {
> +      fs_envblk->ops->write (envblk_on_block);
> +      grub_envblk_close (envblk_on_block);
> +    }
>  }
> 
>  static void
> --
> 2.51.0

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

Re: [PATCH v4 07/12] util/grub-editenv: wire set_variables to optional fs_envblk

[Michael Chang via Grub-devel]

Hi Avnish,

On Thu, Oct 09, 2025 at 02:13:00PM +0530, Avnish Chouhan wrote:
> On 2025-10-09 12:50, grub-devel-request@gnu.org wrote:
> 
> Reviewed-by: Avnish Chouhan <avnish@linux.ibm.com>

Many thanks for your help in reviewing the patches. I'll add your
Reviewed-by tag in next version.

Regards,
Michael

> 
> > Message: 3
> > Date: Thu,  9 Oct 2025 15:18: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 v4 07/12] util/grub-editenv: wire set_variables to
> > 	optional fs_envblk
> > Message-ID: <20251009071845.318828-8-mchang@suse.com>
> > 
> > This patch changes set_variables() so that it can use an external
> > environment block when one is present. The variable next_entry is
> > written into the external block, env_block is treated as read only, and
> > all other variables are written into the normal file based envblk.
> > 
> > A cleanup step is added to handle cases where GRUB at runtime writes
> > variables into the external block because file based updates are not
> > safe on a copy on write filesystem such as Btrfs. For example, the
> > savedefault command can update saved_entry, and on Btrfs GRUB will place
> > that update in the external block instead of the file envblk. If an
> > older copy remains in the external block, it would override the newer
> > value from the file envblk when GRUB first loads the file and then
> > applies the external block on top of it. To avoid this, whenever a
> > variable is updated in the file envblk, any same named key in the
> > external block is deleted.
> > 
> > Signed-off-by: Michael Chang <mchang@suse.com>
> > Reviewed-by: Neal Gompa <ngompa13@gmail.com>
> > ---
> >  util/grub-editenv.c | 58 +++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 56 insertions(+), 2 deletions(-)
> > 
> > diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> > index 42a0c63f8..beb2dd4fc 100644
> > --- a/util/grub-editenv.c
> > +++ b/util/grub-editenv.c
> > @@ -402,12 +402,35 @@ fs_envblk_write (grub_envblk_t envblk)
> >    fclose (fp);
> >  }
> > 
> > +struct var_lookup_ctx {
> > +  const char *varname;
> > +  bool found;
> > +};
> > +typedef struct var_lookup_ctx var_lookup_ctx_t;
> > +
> > +static int
> > +var_lookup_iter (const char *varname, const char *value __attribute__
> > ((unused)), void *hook_data)
> > +{
> > +  var_lookup_ctx_t *ctx = (var_lookup_ctx_t *) hook_data;
> > +
> > +  if (grub_strcmp (ctx->varname, varname) == 0)
> > +    {
> > +      ctx->found = true;
> > +      return 1;
> > +    }
> > +  return 0;
> > +}
> > +
> >  static void
> >  set_variables (const char *name, int argc, char *argv[])
> >  {
> >    grub_envblk_t envblk;
> > +  grub_envblk_t envblk_on_block = NULL;
> > 
> >    envblk = open_envblk_file (name);
> > +  if (fs_envblk != NULL)
> > +    envblk_on_block = fs_envblk->ops->open (envblk);
> > +
> >    while (argc)
> >      {
> >        char *p;
> > @@ -418,15 +441,46 @@ set_variables (const char *name, int argc, char
> > *argv[])
> > 
> >        *(p++) = 0;
> > 
> > -      if (! grub_envblk_set (envblk, argv[0], p))
> > -        grub_util_error ("%s", _("environment block too small"));
> > +      if ((strcmp (argv[0], "next_entry") == 0) && envblk_on_block !=
> > NULL)
> > +	{
> > +	  if (grub_envblk_set (envblk_on_block, argv[0], p) == 0)
> > +	    grub_util_error ("%s", _("environment block too small"));
> > +	  goto next;
> > +	}
> > +
> > +      if (strcmp (argv[0], "env_block") == 0)
> > +	{
> > +	  grub_util_warn (_("can't set env_block as it's read-only"));
> > +	  goto next;
> > +	}
> > +
> > +      if (grub_envblk_set (envblk, argv[0], p) == 0)
> > +	grub_util_error ("%s", _("environment block too small"));
> > 
> > +      if (envblk_on_block != NULL)
> > +	{
> > +	  var_lookup_ctx_t ctx = {
> > +	    .varname = argv[0],
> > +	    .found = false
> > +	  };
> > +
> > +	  grub_envblk_iterate (envblk_on_block, &ctx, var_lookup_iter);
> > +	  if (ctx.found == true)
> > +	    grub_envblk_delete (envblk_on_block, argv[0]);
> > +	}
> > + next:
> >        argc--;
> >        argv++;
> >      }
> > 
> >    write_envblk (name, envblk);
> >    grub_envblk_close (envblk);
> > +
> > +  if (envblk_on_block != NULL)
> > +    {
> > +      fs_envblk->ops->write (envblk_on_block);
> > +      grub_envblk_close (envblk_on_block);
> > +    }
> >  }
> > 
> >  static void
> > --
> > 2.51.0

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

[PATCH v4 00/12] 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.

The first two patches are new in this series as feedback from the review
to use a portable string specifier for size_t. Unfortunately the z
length modifier is missing in GRUB printf routine's format specifier.
The new patches add support and test case for that. In theory they can
be reviewed separately.

And also yet another new patch "btrfs: update doc link for bootloader
support" added at the end to address v3 review feedback.

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
v3:
- Replace !fp with explicit (fp == NULL) for clarity
- Fix missing blank line before return statement
- Fix missing space after cast operator
v4:
- Rename ENV_BTRFS_OFFSET to GRUB_ENV_BTRFS_OFFSET
- Move GRUB_ENV_BTRFS_OFFSET into "btrfs: add environment block to
  reserved header area"
- Adjust patch order to resolve relocated GRUB_ENV_BTRFS_OFFSET
- Add patch "btrfs: update doc link for bootloader support"
- Do better typedef
- Add heads-up comments to both fs_envblk_spec and btrfs_head that they
  have to stay in sync
- Fix a few places to use explicit checks for NULL, true/false etc
- Use goto labels to cleanup consistently
- Mark function names with trailing "()" in commits and comments
- Rename envblk_fs to envblk_on_block for better clarity
- Avoid redundant curly braces in if .. else if clause
- Highlight overflow guard around envioronment block in embed_region
- Rename a few functions for clarity
- Use portable printf formats for off_t and size_t
- Support z length modifier in GRUB printf format string
- Add z modifier to printf unit test

Michael Chang (12):
  misc: add z length modifier support
  tests: add z modifier printf tests
  btrfs: add environment block to reserved header area
  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
  00_header.in: wire grub.cfg to use env_block when present
  docs: add Btrfs env block and special env vars
  btrfs: update doc link for bootloader support

 docs/grub.texi           |  60 ++++++
 grub-core/fs/btrfs.c     |  12 +-
 grub-core/kern/misc.c    |  20 ++
 include/grub/fs.h        |   2 +
 tests/printf_unit_test.c |  10 +
 util/grub-editenv.c      | 407 ++++++++++++++++++++++++++++++++++++++-
 util/grub.d/00_header.in |  26 ++-
 7 files changed, 528 insertions(+), 9 deletions(-)

-- 
2.51.0


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

[PATCH v4 07/12] util/grub-editenv: wire set_variables to optional fs_envblk

[Michael Chang via Grub-devel]

This patch changes set_variables() so that it can use an external
environment block when one is present. The variable next_entry is
written into the external block, env_block is treated as read only, and
all other variables are written into the normal file based envblk.

A cleanup step is added to handle cases where GRUB at runtime writes
variables into the external block because file based updates are not
safe on a copy on write filesystem such as Btrfs. For example, the
savedefault command can update saved_entry, and on Btrfs GRUB will place
that update in the external block instead of the file envblk. If an
older copy remains in the external block, it would override the newer
value from the file envblk when GRUB first loads the file and then
applies the external block on top of it. To avoid this, whenever a
variable is updated in the file envblk, any same named key in the
external block is deleted.

Signed-off-by: Michael Chang <mchang@suse.com>
Reviewed-by: Neal Gompa <ngompa13@gmail.com>
---
 util/grub-editenv.c | 58 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/util/grub-editenv.c b/util/grub-editenv.c
index 42a0c63f8..beb2dd4fc 100644
--- a/util/grub-editenv.c
+++ b/util/grub-editenv.c
@@ -402,12 +402,35 @@ fs_envblk_write (grub_envblk_t envblk)
   fclose (fp);
 }
 
+struct var_lookup_ctx {
+  const char *varname;
+  bool found;
+};
+typedef struct var_lookup_ctx var_lookup_ctx_t;
+
+static int
+var_lookup_iter (const char *varname, const char *value __attribute__ ((unused)), void *hook_data)
+{
+  var_lookup_ctx_t *ctx = (var_lookup_ctx_t *) hook_data;
+
+  if (grub_strcmp (ctx->varname, varname) == 0)
+    {
+      ctx->found = true;
+      return 1;
+    }
+  return 0;
+}
+
 static void
 set_variables (const char *name, int argc, char *argv[])
 {
   grub_envblk_t envblk;
+  grub_envblk_t envblk_on_block = NULL;
 
   envblk = open_envblk_file (name);
+  if (fs_envblk != NULL)
+    envblk_on_block = fs_envblk->ops->open (envblk);
+
   while (argc)
     {
       char *p;
@@ -418,15 +441,46 @@ set_variables (const char *name, int argc, char *argv[])
 
       *(p++) = 0;
 
-      if (! grub_envblk_set (envblk, argv[0], p))
-        grub_util_error ("%s", _("environment block too small"));
+      if ((strcmp (argv[0], "next_entry") == 0) && envblk_on_block != NULL)
+	{
+	  if (grub_envblk_set (envblk_on_block, argv[0], p) == 0)
+	    grub_util_error ("%s", _("environment block too small"));
+	  goto next;
+	}
+
+      if (strcmp (argv[0], "env_block") == 0)
+	{
+	  grub_util_warn (_("can't set env_block as it's read-only"));
+	  goto next;
+	}
+
+      if (grub_envblk_set (envblk, argv[0], p) == 0)
+	grub_util_error ("%s", _("environment block too small"));
 
+      if (envblk_on_block != NULL)
+	{
+	  var_lookup_ctx_t ctx = {
+	    .varname = argv[0],
+	    .found = false
+	  };
+
+	  grub_envblk_iterate (envblk_on_block, &ctx, var_lookup_iter);
+	  if (ctx.found == true)
+	    grub_envblk_delete (envblk_on_block, argv[0]);
+	}
+ next:
       argc--;
       argv++;
     }
 
   write_envblk (name, envblk);
   grub_envblk_close (envblk);
+
+  if (envblk_on_block != NULL)
+    {
+      fs_envblk->ops->write (envblk_on_block);
+      grub_envblk_close (envblk_on_block);
+    }
 }
 
 static void
-- 
2.51.0


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