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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/11/14 14:14, Ian Campbell wrote:
> On Fri, 2014-11-21 at 13:32 +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.
> Is someone going to resubmit a patch along these lines then?
>
> Ian
>
>

Unless you are NAKing my original patch, no.

That is untested, and IMO only a gross hack around the complete type
brokenness introduced by d1b93ea.

I still firmly believe that my patch is the proper solution, which
brings all the types back in line.

~Andrew


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