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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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

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