From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH for-4.5 RFC] pygrub: Fix regression from c/s d1b93ea, attempt 2 Date: Mon, 17 Nov 2014 11:41:59 -0500 Message-ID: <546A2557.7010300@oracle.com> References: <1416237586-17785-1-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1416237586-17785-1-git-send-email-andrew.cooper3@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , Xen-devel Cc: Wei Liu , Ian Jackson , Ian Campbell List-Id: xen-devel@lists.xenproject.org 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 > CC: Ian Campbell > CC: Ian Jackson > CC: Wei Liu > CC: Konrad Rzeszutek Wilk > CC: Boris Ostrovsky > > --- > > 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)