[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH for-4.5] pygrub: Fix regression from c/s d1b93ea, attempt 2
On Tue, 2014-12-02 at 02:43 +0000, Andrew Cooper wrote: > On 01/12/2014 20:30, Konrad Rzeszutek Wilk wrote: > > On Fri, Nov 28, 2014 at 11:31:24AM +0000, Ian Campbell wrote: > >> On Tue, 2014-11-25 at 11:11 -0500, Boris Ostrovsky wrote: > >>> c/s d1b93ea causes substantial functional regressions in pygrub's ability > >>> to > >>> parse bootloader configuration files. > >>> > >>> c/s d1b93ea itself changed an an interface which previously used > >>> exclusively > >>> integers, to using strings in the case of a grub configuration with > >>> explicit > >>> default set, along with changing the code calling the interface to > >>> require a > >>> string. The default value for "default" remained as an integer. > >>> > >>> As a result, any Extlinux or Lilo configuration (which drives this > >>> interface > >>> exclusively with integers), or Grub configuration which doesn't explicitly > >>> declare a default will die with an AttributeError when attempting to call > >>> "self.cf.default.isdigit()" where "default" is an integer. > >>> > >>> Sadly, this AttributeError gets swallowed by the blanket ignore in the > >>> loop > >>> which searches partitions for valid bootloader configurations, causing the > >>> issue to be reported as "Unable to find partition containing kernel" > >>> > >>> We should explicitly check type of "default" in image_index() and process > >>> it > >>> appropriately. > >>> > >>> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > >>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> > >> Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx> > >> > >> In general I would prefer the first line of the commit message to be a > >> short description of the actual functional change and not a reference to > >> fixing some other commit, which is basically meaningless to anyone > >> reading the log even now, nevermind in six months. I think I'm going to > >> start picking up on this more frequently from now on. > >> > >> CCing Konrad for RM input. I'm much less worried about corner cases etc > >> wrt the freeze etc than I was with the larger fix proposed earlier. > > I am bit lost. I thought Andrew NACKed this? > > I didn't, as I am not in a position to. I have not tested the result, > but believe it is sufficient to fix the exact error at hand. If the > maintainers think it is the best solution then so be it, but I am still > of the opinion that it is is still a hack upon a hack. At this point in a freeze I'm much happier with: tools/pygrub/src/pygrub | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) than tools/pygrub/src/ExtLinuxConf.py | 6 +++--- tools/pygrub/src/GrubConf.py | 7 ++----- tools/pygrub/src/LiloConf.py | 6 +++--- 3 files changed, 8 insertions(+), 11 deletions(-) Plus Boris' patch is far easier to reason about in isolation in a dynamically/duck typed language. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |