[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.