Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Steven Haigh writes ("Re: [Xen-devel] [PATCH] read grubenv and set default from 
saved_entry or next_entry"):
> Just wanted to give this a quick followup... Did this end up 
> progressing?

Hi.  I'm a tools maintainer and probably your best bet for a review
etc of this patch.  If, next time, you use add_maintainers.pl or
something, you'll end up CCing the maintainer and your mail won't get
dropped.  Anyway, thanks for chasing it up through a back channel :-).

Michael Young <m.a.young@xxxxxxxxxxxx>:
> From 51a9dce9de3ea159011928e2db8541f3c7e8383a Mon Sep 17 00:00:00 2001
> From: Michael Young <m.a.young@xxxxxxxxxxxx>
> Date: Thu, 15 Aug 2019 19:55:30 +0100
> Subject: [PATCH] read grubenv and set default from saved_entry or next_entry
> 
> This patch looks for a grubenv file in the same directory as the
> grub.cfg file and includes it at front of the grub.cfg file when passed
> to parse()

Thanks for the contribution.  I reviewed the patch and I have some
comments.

I think this patch would be less confusing if it were two patches.
One which does the saved/next entry, and one which reads grubenv.  Do
you think that would make sense ?  If so I would appreciate it if you
would split it up (and write a nice explanatory commit message about
the saved_entry stuff).

> As the grubenv file consists of variable=value lines padded by hashes these
> are treated as commands in parse() where it uses the value of saved_entry
> or next_entry (if set) to set the default entry if a title matches or is
> a number.

I like reusing the parser.

> diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> index ce7ab0eb8c..267788795b 100755
> --- a/tools/pygrub/src/pygrub
> +++ b/tools/pygrub/src/pygrub
> @@ -454,8 +454,19 @@ class Grub:
>          if self.__dict__.get('cf', None) is None:
>              raise RuntimeError("couldn't find bootloader config file in the 
> image provided.")
>          f = fs.open_file(self.cf.filename)
> +        fenv = self.cf.filename.replace("grub.cfg","grubenv")

I find this filename hackery rather unprincipled.  I'm not entirely
sure I can see a better way, given the way cfg_list is constructed.
Can you think of a less hacky approach ?

What would happen in future if we provided a way to control the
grub.cfg read by pygrub and a user configured it to (say)
`grub.cfg.old' ?  Would we really want it to read `grubenv.old' ?

> +        if fenv != self.cf.filename and fs.file_exists(fenv):
> +            # if grubenv file exists next to grub.cfg prepend it
> +            fenvf = fs.open_file(fenv)
> +            if sys.version_info[0] < 3:
> +                fsep = "\n"
> +            else:
> +                fsep = b"\n"
> +            buf = fsep.join((fenvf.read(FS_READ_MAX),f.read(FS_READ_MAX)))
> +            del fenvf
>          # limit read size to avoid pathological cases
> -        buf = f.read(FS_READ_MAX)
> +        else:
> +            buf = f.read(FS_READ_MAX)
>          del f
>          if sys.version_info[0] < 3:
>              self.cf.parse(buf)

Can we instead make the parser take a list ?  This business of
constructing a concatenated string is not very nice.

Regards,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel