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 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 <>:
> From 51a9dce9de3ea159011928e2db8541f3c7e8383a Mon Sep 17 00:00:00 2001
> From: Michael Young <>
> 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

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(
> +        fenv ="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 != 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((,
> +            del fenvf
>          # limit read size to avoid pathological cases
> -        buf =
> +        else:
> +            buf =
>          del f
>          if sys.version_info[0] < 3:

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


Xen-devel mailing list

Reply via email to