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

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

On 17/11/14 16:41, Boris Ostrovsky wrote:
> On 11/17/2014 10:19 AM, 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.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>
>> ---
>>
>> This patch is RFC because, while I have dev tested and proved that it
>> unwedges
>> the specific senario I encountered, I have not yet tested that it
>> contines to
>> boot all other PV guests.
>>
>> As for 4.5-ness, this is a must-fix as far as I am concerned
>>
>> Either:
>>   1) Revert d1b93ea (original bad changeset) and 4ee393f (attempt 1
>> to fix)
>>   2) Take this patch in addition which hopefully fixes the regressions
>> ---
>>   tools/pygrub/src/ExtLinuxConf.py |    6 +++---
>>   tools/pygrub/src/GrubConf.py     |    7 ++-----
>>   tools/pygrub/src/LiloConf.py     |    6 +++---
>>   3 files changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/tools/pygrub/src/ExtLinuxConf.py
>> b/tools/pygrub/src/ExtLinuxConf.py
>> index 510099b..e70fca6 100644
>> --- a/tools/pygrub/src/ExtLinuxConf.py
>> +++ b/tools/pygrub/src/ExtLinuxConf.py
>> @@ -123,7 +123,7 @@ class ExtLinuxConfigFile(object):
>>           self.filename = fn
>>           self.images = []
>>           self.timeout = -1
>> -        self._default = 0
>> +        self._default = "0"
>>             if fn is not None:
>>               self.parse()
>> @@ -191,8 +191,8 @@ class ExtLinuxConfigFile(object):
>>       def _get_default(self):
>>           for i in range(len(self.images)):
>>               if self.images[i].title == self._default:
>> -                return i
>> -        return 0
>> +                return str(i)
>> +        return "0"
>>       def _set_default(self, val):
>>           self._default = val
>>       default = property(_get_default, _set_default)
>> diff --git a/tools/pygrub/src/GrubConf.py b/tools/pygrub/src/GrubConf.py
>> index dea7044..645b6e2 100644
>> --- a/tools/pygrub/src/GrubConf.py
>> +++ b/tools/pygrub/src/GrubConf.py
>> @@ -170,7 +170,7 @@ class _GrubConfigFile(object):
>>           self.filename = fn
>>           self.images = []
>>           self.timeout = -1
>> -        self._default = 0
>> +        self._default = "0"
>>           self.passwordAccess = True
>>           self.passExc = None
>>   @@ -229,12 +229,9 @@ class _GrubConfigFile(object):
>>           return self._default
>>       def _set_default(self, val):
>>           if val == "saved":
>> -            self._default = 0
>> +            self._default = "0"
>>           else:
>>               self._default = val
>
> If we are using strings-only value, should this also be str(val)? Here
> and elsewhere. (My python skills are highly questionable so I don't
> know whether this would be needed).
>
> Other than that, I tested this with a few grub2 configurations and it
> worked fine.

I believe not, as _set_default() is unconditionally called with val as a
string, in all config parsers.

Observe that you switched int(val) -> val in your original change.

~Andrew