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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Fri, Nov 21, 2014 at 01:32:13PM +0000, Andrew Cooper wrote:
> On 20/11/14 16:45, Boris Ostrovsky wrote:
> > On 11/20/2014 11:15 AM, Ian Campbell wrote:
> >> On Thu, 2014-11-20 at 16:08 +0000, Andrew Cooper wrote:
> >>> On 20/11/14 16:00, Ian Campbell wrote:
> >>>> On Mon, 2014-11-17 at 15:19 +0000, Andrew Cooper 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"
> >>>>>
> >>>>> This patch attempts to fix the issue by altering all parts of this
> >>>>> interface
> >>>>> to use strings, as opposed to integers.
> >>>> Would it be less invasive at this point in the release to have the
> >>>> place
> >>>> where this stuff is used do isinstance(s, str) and isinstance(s, int)?
> >>> It would be BaseString not str, but I am fairly sure the classes have
> >>> altered through Py2's history.  I would not be any more confident with
> >>> that as a solution as trying to correctly to start with.
> >> Actually isinstance(s, basestring) is what the webpage I was looking at
> >> said, but I cut-n-pasted the wrong thing.
> >>
> >> But regardless could it not do something like:
> >>     if !isinstance(foo.cf.default, int):
> >
> > I think it would be the other way around, e.g. (not tested):
> >
> > diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> > index aa7e562..7250f45 100644
> > --- 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 if self.cf.default.isdigit():
> >              sel = int(self.cf.default)
> >          else:
> >              # We don't fully support submenus. Look for the leaf
> > value in
> >
> > but yes, this looks less intrusive (assuming this the only place where
> > we'd hit this error).
> >
> 
> That does look plausibly like it would fix the issue.
> 
> However, I can't help but feeing that this is hacking around a broken
> patch in the first place.

I cannot think of a reason you would ever feel that way!
<surreptitiously checks the roll of Xen 4.5 band-aid>

We know that the existing patches work fine for a host of Fedora
families (15->21) (thought I need to double check that the testing framework
that we have is using pygrub and not pvgrub), SLESs and RHEL5s, and OL6s.

The ones that I am worried about are the ExtLinux and such which
I didn't realize would use 'pygrub'.

Thought I just remembered a bug with OL7 grub entries - that is if
you go in the interactive menu things broke down. Boris, do you
remember if that was fixed or just 'deferred'?

> 
> ~Andrew