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

[Ian Campbell <Ian.Campbell@citrix.com>]

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):
       blah = int(foo.cf.default)
   elif foo.cf.default.isdigit():
       blah = whatever
       
and avoid the confusion about what is/isn't a string class while still
fixing the issue?

> sel comes either from g.image_index() which strictly is an integer, or
> pulled out of the loop immediately preceding and strictly an integer.

Ah, good.

> I can't however find anything which could cause it to have the value
> -1.  All error paths I can spot use 0 instead and load the first entry.
> 
> ~Andrew
>