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

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

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?

> 
> > ---
> > 
> > Commit message is Andrew's with exception of the last sentense.
> > 
> > I only tested this with grub2.
> > 
> > -boris
> > 
> >  tools/pygrub/src/pygrub |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >  mode change 100644 => 100755 tools/pygrub/src/pygrub
> > 
> > diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> > old mode 100644
> > new mode 100755
> > index aa7e562..3ec52fd
> > --- a/tools/pygrub/src/pygrub
> > +++ b/tools/pygrub/src/pygrub
> > @@ -457,7 +457,9 @@ class Grub:
> >          self.cf.parse(buf)
> >  
> >      def image_index(self):
> > -        if self.cf.default.isdigit():
> > +        if isinstance(self.cf.default, int):
> > +            sel = self.cf.default
> > +        elif self.cf.default.isdigit():
> >              sel = int(self.cf.default)
> >          else:
> >              # We don't fully support submenus. Look for the leaf value in
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel