On Fri, Sep 05, 2025 at 10:15:48PM +0200, Daniel Kiper wrote:
> On Tue, Sep 02, 2025 at 02:45:59PM +0800, Michael Chang via Grub-devel wrote:
> > 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]>
> > Reviewed-by: Neal Gompa <[email protected]>
> > ---
> > util/grub-editenv.c | 37 ++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/util/grub-editenv.c b/util/grub-editenv.c
> > index 7bc872dc7..a319d01b7 100644
> > --- a/util/grub-editenv.c
> > +++ b/util/grub-editenv.c
> > @@ -133,11 +133,14 @@ struct fs_envblk_spec {
> > };
> >
> > 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);
> > } fs_envblk_ops = {
> > - .open = fs_envblk_open
> > + .open = fs_envblk_open,
> > + .write = fs_envblk_write
> > };
> >
> > struct fs_envblk {
> > @@ -356,6 +359,38 @@ 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;
> > + int offset, size;
>
> Again, please use proper types for offset and size.
OK. Will do it.
>
> > + 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");
>
> Why do you use "b" here? Environment block is not a binary (file)...
Just in case there are side effects with newline characters on non-POSIX
systems like MS Windows. Also existing grubenv uses "b" when opening
files in write_envblk().
>
> And s/r+/w+/?
IMHO "r+" is more suitable here as it will simply fail if the device
file does not exist and unlike "w+" it will not attempt to create or
truncate the file.
Thanks,
Michael
>
> Daniel
_______________________________________________
Grub-devel mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/grub-devel
