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: Thu, 20 Nov 2014 11:45:47 -0500 Message-ID: <546E1ABB.6090308@oracle.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1416500123.20161.3.camel@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: Ian Campbell , Andrew Cooper Cc: Wei Liu , Ian Jackson , Xen-devel List-Id: xen-devel@lists.xenproject.org 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 >> >