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.yo...@durham.ac.uk>: > From 51a9dce9de3ea159011928e2db8541f3c7e8383a Mon Sep 17 00:00:00 2001 > From: Michael Young <m.a.yo...@durham.ac.uk> > 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 < 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 < 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 Xenfirstname.lastname@example.org https://lists.xenproject.org/mailman/listinfo/xen-devel