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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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.

-boris

> -
> -        if self._default < 0:
> -            raise ValueError, "default must be positive number"
>       default = property(_get_default, _set_default)
>   
>       def set_splash(self, val):
> diff --git a/tools/pygrub/src/LiloConf.py b/tools/pygrub/src/LiloConf.py
> index 2cb649f..53411e6 100644
> --- a/tools/pygrub/src/LiloConf.py
> +++ b/tools/pygrub/src/LiloConf.py
> @@ -89,7 +89,7 @@ class LiloConfigFile(object):
>           self.filename = fn
>           self.images = []
>           self.timeout = -1
> -        self._default = 0
> +        self._default = "0"
>   
>           if fn is not None:
>               self.parse()
> @@ -156,8 +156,8 @@ class LiloConfigFile(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)