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

["YOUNG, MICHAEL A." <m.a.young@xxxxxxxxxxxx>]

On Wed, 11 Sep 2019, Ian Jackson wrote:

> 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).

Yes, it makes sense to split it up, perhaps with a third patch for a 
format change of what is passed from pygrub to the backemds.

>> 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' ?
>

One option would be to do a fresh search for grubenv in the same places
we looked for grub.cfg.
Alternatively, it should be possible to do a more precise edit using
f.rpartition("/").

>> +        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.

Yes a list is probably the way to go, particularly as we might want to 
pass more file contents later for BLS support.

        Michael Young

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