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

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

[Boris Ostrovsky]

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"

We should explicitly check type of "default" in image_index() and process it
appropriately.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---

Commit message is Andrew's with exception of the last sentense.

I only tested this with grub2.

-boris

 tools/pygrub/src/pygrub |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
 mode change 100644 => 100755 tools/pygrub/src/pygrub

diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
old mode 100644
new mode 100755
index aa7e562..3ec52fd
--- 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 self.cf.default.isdigit():
             sel = int(self.cf.default)
         else:
             # We don't fully support submenus. Look for the leaf value in
-- 
1.7.1

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

[Ian Campbell]

On Tue, 2014-11-25 at 11:11 -0500, Boris Ostrovsky 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"
> 
> We should explicitly check type of "default" in image_index() and process it
> appropriately.
> 
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

In general I would prefer the first line of the commit message to be a
short description of the actual functional change and not a reference to
fixing some other commit, which is basically meaningless to anyone
reading the log even now, nevermind in six months. I think I'm going to
start picking up on this more frequently from now on.

CCing Konrad for RM input. I'm much less worried about corner cases etc
wrt the freeze etc than I was with the larger fix proposed earlier.

> ---
> 
> Commit message is Andrew's with exception of the last sentense.
> 
> I only tested this with grub2.
> 
> -boris
> 
>  tools/pygrub/src/pygrub |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
>  mode change 100644 => 100755 tools/pygrub/src/pygrub
> 
> diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> old mode 100644
> new mode 100755
> index aa7e562..3ec52fd
> --- 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 self.cf.default.isdigit():
>              sel = int(self.cf.default)
>          else:
>              # We don't fully support submenus. Look for the leaf value in

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

[Konrad Rzeszutek Wilk]

On Fri, Nov 28, 2014 at 11:31:24AM +0000, Ian Campbell wrote:
> On Tue, 2014-11-25 at 11:11 -0500, Boris Ostrovsky 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"
> > 
> > We should explicitly check type of "default" in image_index() and process it
> > appropriately.
> > 
> > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> In general I would prefer the first line of the commit message to be a
> short description of the actual functional change and not a reference to
> fixing some other commit, which is basically meaningless to anyone
> reading the log even now, nevermind in six months. I think I'm going to
> start picking up on this more frequently from now on.
> 
> CCing Konrad for RM input. I'm much less worried about corner cases etc
> wrt the freeze etc than I was with the larger fix proposed earlier.

I am bit lost. I thought Andrew NACKed this?

> 
> > ---
> > 
> > Commit message is Andrew's with exception of the last sentense.
> > 
> > I only tested this with grub2.
> > 
> > -boris
> > 
> >  tools/pygrub/src/pygrub |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> >  mode change 100644 => 100755 tools/pygrub/src/pygrub
> > 
> > diff --git a/tools/pygrub/src/pygrub b/tools/pygrub/src/pygrub
> > old mode 100644
> > new mode 100755
> > index aa7e562..3ec52fd
> > --- 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 self.cf.default.isdigit():
> >              sel = int(self.cf.default)
> >          else:
> >              # We don't fully support submenus. Look for the leaf value in
> 
> 

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

[Andrew Cooper]

On 01/12/2014 20:30, Konrad Rzeszutek Wilk wrote:
> On Fri, Nov 28, 2014 at 11:31:24AM +0000, Ian Campbell wrote:
>> On Tue, 2014-11-25 at 11:11 -0500, Boris Ostrovsky 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"
>>>
>>> We should explicitly check type of "default" in image_index() and process it
>>> appropriately.
>>>
>>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>
>> In general I would prefer the first line of the commit message to be a
>> short description of the actual functional change and not a reference to
>> fixing some other commit, which is basically meaningless to anyone
>> reading the log even now, nevermind in six months. I think I'm going to
>> start picking up on this more frequently from now on.
>>
>> CCing Konrad for RM input. I'm much less worried about corner cases etc
>> wrt the freeze etc than I was with the larger fix proposed earlier.
> I am bit lost. I thought Andrew NACKed this?

I didn't, as I am not in a position to.  I have not tested the result,
but believe it is sufficient to fix the exact error at hand.  If the
maintainers think it is the best solution then so be it, but I am still
of the opinion that it is is still a hack upon a hack.

~Andrew

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

[Ian Campbell]

On Tue, 2014-12-02 at 02:43 +0000, Andrew Cooper wrote:
> On 01/12/2014 20:30, Konrad Rzeszutek Wilk wrote:
> > On Fri, Nov 28, 2014 at 11:31:24AM +0000, Ian Campbell wrote:
> >> On Tue, 2014-11-25 at 11:11 -0500, Boris Ostrovsky 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"
> >>>
> >>> We should explicitly check type of "default" in image_index() and process it
> >>> appropriately.
> >>>
> >>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >>
> >> In general I would prefer the first line of the commit message to be a
> >> short description of the actual functional change and not a reference to
> >> fixing some other commit, which is basically meaningless to anyone
> >> reading the log even now, nevermind in six months. I think I'm going to
> >> start picking up on this more frequently from now on.
> >>
> >> CCing Konrad for RM input. I'm much less worried about corner cases etc
> >> wrt the freeze etc than I was with the larger fix proposed earlier.
> > I am bit lost. I thought Andrew NACKed this?
> 
> I didn't, as I am not in a position to.  I have not tested the result,
> but believe it is sufficient to fix the exact error at hand.  If the
> maintainers think it is the best solution then so be it, but I am still
> of the opinion that it is is still a hack upon a hack.

At this point in a freeze I'm much happier with:

 tools/pygrub/src/pygrub |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

than
 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(-)

Plus Boris' patch is far easier to reason about in isolation in a
dynamically/duck typed language.

Ian.

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

[Konrad Rzeszutek Wilk]

On Tue, Dec 02, 2014 at 02:02:58PM +0000, Ian Campbell wrote:
> On Tue, 2014-12-02 at 02:43 +0000, Andrew Cooper wrote:
> > On 01/12/2014 20:30, Konrad Rzeszutek Wilk wrote:
> > > On Fri, Nov 28, 2014 at 11:31:24AM +0000, Ian Campbell wrote:
> > >> On Tue, 2014-11-25 at 11:11 -0500, Boris Ostrovsky 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"
> > >>>
> > >>> We should explicitly check type of "default" in image_index() and process it
> > >>> appropriately.
> > >>>
> > >>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > >>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > >> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > >>
> > >> In general I would prefer the first line of the commit message to be a
> > >> short description of the actual functional change and not a reference to
> > >> fixing some other commit, which is basically meaningless to anyone
> > >> reading the log even now, nevermind in six months. I think I'm going to
> > >> start picking up on this more frequently from now on.
> > >>
> > >> CCing Konrad for RM input. I'm much less worried about corner cases etc
> > >> wrt the freeze etc than I was with the larger fix proposed earlier.
> > > I am bit lost. I thought Andrew NACKed this?
> > 
> > I didn't, as I am not in a position to.  I have not tested the result,
> > but believe it is sufficient to fix the exact error at hand.  If the
> > maintainers think it is the best solution then so be it, but I am still
> > of the opinion that it is is still a hack upon a hack.
> 
> At this point in a freeze I'm much happier with:
> 
>  tools/pygrub/src/pygrub |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)

The same here. This is now the season of handing out band-aids.

As such Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> than
>  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(-)
> 
> Plus Boris' patch is far easier to reason about in isolation in a
> dynamically/duck typed language.
> 
> Ian.
> 

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

[Ian Campbell]

On Tue, 2014-12-02 at 10:09 -0500, Konrad Rzeszutek Wilk wrote:
> > At this point in a freeze I'm much happier with:
> > 
> >  tools/pygrub/src/pygrub |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> The same here. This is now the season of handing out band-aids.
> 
> As such Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Applied.

> > 
> > than
> >  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(-)
> > 
> > Plus Boris' patch is far easier to reason about in isolation in a
> > dynamically/duck typed language.
> > 
> > Ian.
> > 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel