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

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

[Andrew Cooper]

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 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)
-- 
1.7.10.4

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

[Boris Ostrovsky]

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)

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

[Andrew Cooper]

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

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

[Ian Campbell]

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.

Please can you and Boris both provide examples of (ideally real-world)
configuration files which exhibit these failures as patches against
tools/pygrub/examples/.

Boris, in your case I mean the one which caused you to write the
original patch, which I should have remembered to ask for at the time.

Andy, in your case it would make sense to include at least one in this
patch I think.

Ian.

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

[Andrew Cooper]

On 17/11/14 16:58, 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.
> Please can you and Boris both provide examples of (ideally real-world)
> configuration files which exhibit these failures as patches against
> tools/pygrub/examples/.
>
> Boris, in your case I mean the one which caused you to write the
> original patch, which I should have remembered to ask for at the time.
>
> Andy, in your case it would make sense to include at least one in this
> patch I think.
>
> Ian.
>

examples/ doesn't help.  The parsers themselves don't exhibit the bug. 
It is only when pygrub itself attempts to interact with the parsed
config does the issue exhibits itself.

I would have provided an example if it would have helped.

~Andrew

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

[Boris Ostrovsky]

On 11/17/2014 11:59 AM, Andrew Cooper wrote:
> On 17/11/14 16:58, 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.
>> Please can you and Boris both provide examples of (ideally real-world)
>> configuration files which exhibit these failures as patches against
>> tools/pygrub/examples/.
>>
>> Boris, in your case I mean the one which caused you to write the
>> original patch, which I should have remembered to ask for at the time.


I wanted to be able to parse grub2's default values set as strings, such as

    set default="Fedora, with Xen xen and Linux 3.17.0-rc3"

or, for submenus

    set default="Advanced options for Fedora (with Xen 
hypervisor)>Fedora, with Xen xen and Linux 3.17.0-rc3"

Original pygrub would not be able to understand the string and default 
to zero.

-boris


>>
>> Andy, in your case it would make sense to include at least one in this
>> patch I think.
>>
>> Ian.
>>
> examples/ doesn't help.  The parsers themselves don't exhibit the bug.
> It is only when pygrub itself attempts to interact with the parsed
> config does the issue exhibits itself.
>
> I would have provided an example if it would have helped.
>
> ~Andrew

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

[Ian Campbell]

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)?

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.

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

[Andrew Cooper]

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

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

[Ian Campbell]

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):
       blah = int(foo.cf.default)
   elif foo.cf.default.isdigit():
       blah = whatever
       
and avoid the confusion about what is/isn't a string class while still
fixing the issue?

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

Ah, good.

> 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
> 

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

[Boris Ostrovsky]

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).


-boris


>         blah = int(foo.cf.default)
>     elif foo.cf.default.isdigit():
>         blah = whatever
>         
> and avoid the confusion about what is/isn't a string class while still
> fixing the issue?
>
>> sel comes either from g.image_index() which strictly is an integer, or
>> pulled out of the loop immediately preceding and strictly an integer.
> Ah, good.
>
>> 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
>>
>

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

[Andrew Cooper]

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

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

[Konrad Rzeszutek Wilk]

On Fri, Nov 21, 2014 at 01:32:13PM +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.
> 
> However, I can't help but feeing that this is hacking around a broken
> patch in the first place.

I cannot think of a reason you would ever feel that way!
<surreptitiously checks the roll of Xen 4.5 band-aid>

We know that the existing patches work fine for a host of Fedora
families (15->21) (thought I need to double check that the testing framework
that we have is using pygrub and not pvgrub), SLESs and RHEL5s, and OL6s.

The ones that I am worried about are the ExtLinux and such which
I didn't realize would use 'pygrub'.

Thought I just remembered a bug with OL7 grub entries - that is if
you go in the interactive menu things broke down. Boris, do you
remember if that was fixed or just 'deferred'?

> 
> ~Andrew

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

[Boris Ostrovsky]

On 11/21/2014 12:09 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Nov 21, 2014 at 01:32:13PM +0000, Andrew Cooper wrote:
>> 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.
> I cannot think of a reason you would ever feel that way!
> <surreptitiously checks the roll of Xen 4.5 band-aid>
>
> We know that the existing patches work fine for a host of Fedora
> families (15->21) (thought I need to double check that the testing framework
> that we have is using pygrub and not pvgrub), SLESs and RHEL5s, and OL6s.
>
> The ones that I am worried about are the ExtLinux and such which
> I didn't realize would use 'pygrub'.
>
> Thought I just remembered a bug with OL7 grub entries - that is if
> you go in the interactive menu things broke down. Boris, do you
> remember if that was fixed or just 'deferred'?

The only bug that I remember that had to do pygrub and OL7 was the fact 
that pygrub cannot parse grubenv (and therefore it's not really 
OL7-specific).

We decided not to fix it.


-boris

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

[Ian Campbell]

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

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

[Andrew Cooper]

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