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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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.

By far the least risky option at this point would be to revert the two
identified commits in the comments of the original patch.

>
> Also, in run_grub sel can be set to g.cf.default and then:
>     if sel == -1:
>         print "No kernel image selected!"
>         sys.exit(1)
>
> I can't see where the -1 comes from though, and you aren't changing any
> -1 into "-1", so maybe something more subtle is going on there.
>
> Ian.
>
>

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

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