Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]
Lars Kurth writes ("Re: [Xen-devel] [PATCH] read grubenv and set default from
saved_entry or next_entry [and 1 more messages]"):
> I am assuming there is no chance that this will make 4.13 with the freeze
> having passed.
Assuming we can get good quality patches, I would probably support a
freeze exception. Failing that, this would be a strong candidate for
a backport.
> But in any case, I wasn't sure whether Michael strictly will need it
> as the change can presumably be carried in a fedora patch q for Xen
> packages until it ends up upstream
That's not really enough, because pygrub runs in the host but
interprets the guest filesystem. Since we want to be able to boot
Fedora guests on non-Fedora hosts, that means that every host must
have this fix.
This is an inherent consequence of pygrub's design approach, and means
that I aggressively backport pygrub changes to older releases, so that
older hosts can boot newer guests.
But in the meantime putting this patch in the Fedora host packages is
not a terrible idea. (There is a bit of a risk that if we deploy in
Fedora one algorith, and then upstream a different algorithm, we may
end up stuck with divergence, but I doubt this will be a big problem
here.)
thanks,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]
> On 7 Oct 2019, at 11:01, Ian Jackson wrote:
>
> Hi. Thanks for the message.
>
> Just one thing I wanted to reply to in your mail:
>
> YOUNG, MICHAEL A. writes ("Re: [Xen-devel] [PATCH] read grubenv and set
> default from saved_entry or next_entry [and 1 more messages]"):
>> On Wed, 11 Sep 2019, Ian Jackson wrote:
>>> 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 ?
> ...
>> 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("/").
>
> I don't feel I fully understand the implications, but either of these
> seems like they might be good strategies to me. I guess the former
> risks finding an unrelated leftover grubenv somewhere which is
> probably not desirable.
>
> Anyway, I will leave this to your judgement.
>
> Thanks for the rest of your comments. I'll look forward to a revised
> set of patches - thanks.
Hi all,
I am assuming there is no chance that this will make 4.13 with the freeze
having passed.
But in any case, I wasn't sure whether Michael strictly will need it as the
change can presumably be carried in a fedora patch q for Xen packages until it
ends up upstream
Lars
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]
Hi. Thanks for the message.
Just one thing I wanted to reply to in your mail:
YOUNG, MICHAEL A. writes ("Re: [Xen-devel] [PATCH] read grubenv and set default
from saved_entry or next_entry [and 1 more messages]"):
> On Wed, 11 Sep 2019, Ian Jackson wrote:
> > 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 ?
...
> 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("/").
I don't feel I fully understand the implications, but either of these
seems like they might be good strategies to me. I guess the former
risks finding an unrelated leftover grubenv somewhere which is
probably not desirable.
Anyway, I will leave this to your judgement.
Thanks for the rest of your comments. I'll look forward to a revised
set of patches - thanks.
Regards,
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]
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 :
>> 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
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] read grubenv and set default from saved_entry or next_entry [and 1 more messages]
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 :
> 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
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
