From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH for-4.5 RFC] pygrub: Fix regression from c/s d1b93ea, attempt 2 Date: Fri, 21 Nov 2014 12:09:33 -0500 Message-ID: <20141121170933.GB8314@laptop.dumpdata.com> References: <1416237586-17785-1-git-send-email-andrew.cooper3@citrix.com> <1416499238.14429.39.camel@citrix.com> <546E1206.5080403@citrix.com> <1416500123.20161.3.camel@citrix.com> <546E1ABB.6090308@oracle.com> <546F3EDD.3020006@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <546F3EDD.3020006@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 Cc: Wei Liu , Boris Ostrovsky , Ian Jackson , Ian Campbell , Xen-devel List-Id: xen-devel@lists.xenproject.org On Fri, Nov 21, 2014 at 01:32:13PM +0000, Andrew Cooper wrote: > On 20/11/14 16:45, Boris Ostrovsky wrote: > > 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). > > > > That does look plausibly like it would fix the issue. > > However, I can't help but feeing that this is hacking around a broken > patch in the first place. I cannot think of a reason you would ever feel that way! We know that the existing patches work fine for a host of Fedora families (15->21) (thought I need to double check that the testing framework that we have is using pygrub and not pvgrub), SLESs and RHEL5s, and OL6s. The ones that I am worried about are the ExtLinux and such which I didn't realize would use 'pygrub'. Thought I just remembered a bug with OL7 grub entries - that is if you go in the interactive menu things broke down. Boris, do you remember if that was fixed or just 'deferred'? > > ~Andrew