Re: [Xen-devel] [PATCH for-4.5] pygrub: Fix regression from c/s d1b93ea, attempt 2

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Dec 02, 2014 at 02:02:58PM +0000, Ian Campbell wrote:
> 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(-)

The same here. This is now the season of handing out band-aids.

As such Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> 
> 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