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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

~Andrew

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