From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH for-4.5 RFC] pygrub: Fix regression from c/s d1b93ea, attempt 2 Date: Thu, 20 Nov 2014 16:15:23 +0000 Message-ID: <1416500123.20161.3.camel@citrix.com> References: <1416237586-17785-1-git-send-email-andrew.cooper3@citrix.com> <1416499238.14429.39.camel@citrix.com> <546E1206.5080403@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <546E1206.5080403@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 , Xen-devel List-Id: xen-devel@lists.xenproject.org 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): 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 >