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, [email protected] wrote: > > Message: 4 > > Date: Mon, 15 Sep 2025 17:08:42 +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 3/9] util/grub-editenv: add fs_envblk write helper > > Message-ID: <[email protected]> > > > > 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 <[email protected]> > > --- > > 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 [email protected] https://lists.gnu.org/mailman/listinfo/grub-devel
