[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/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@xxxxxxxxxx>
CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
CC: Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>
CC: Wei Liu <wei.liu2@xxxxxxxxxx>
CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
CC: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>

---

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)


_______________________________________________
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®.