From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH for-4.5] pygrub: Fix regression from c/s d1b93ea, attempt 2 Date: Tue, 2 Dec 2014 10:09:33 -0500 Message-ID: <20141202150933.GE27869@laptop.dumpdata.com> References: <1416931910-8222-1-git-send-email-boris.ostrovsky@oracle.com> <1417174284.23604.9.camel@citrix.com> <20141201203001.GE21626@laptop.dumpdata.com> <547D276C.2070805@citrix.com> <1417528978.24320.42.camel@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: <1417528978.24320.42.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 Cc: Andrew Cooper , Boris Ostrovsky , Ian.Jackson@eu.citrix.com, wei.liu2@citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Tue, Dec 02, 2014 at 02:02:58PM +0000, Ian Campbell wrote: > On Tue, 2014-12-02 at 02:43 +0000, Andrew Cooper wrote: > > On 01/12/2014 20:30, Konrad Rzeszutek Wilk wrote: > > > On Fri, Nov 28, 2014 at 11:31:24AM +0000, Ian Campbell wrote: > > >> On Tue, 2014-11-25 at 11:11 -0500, Boris Ostrovsky 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" > > >>> > > >>> We should explicitly check type of "default" in image_index() and process it > > >>> appropriately. > > >>> > > >>> Reported-by: Andrew Cooper > > >>> Signed-off-by: Boris Ostrovsky > > >> Acked-by: Ian Campbell > > >> > > >> In general I would prefer the first line of the commit message to be a > > >> short description of the actual functional change and not a reference to > > >> fixing some other commit, which is basically meaningless to anyone > > >> reading the log even now, nevermind in six months. I think I'm going to > > >> start picking up on this more frequently from now on. > > >> > > >> CCing Konrad for RM input. I'm much less worried about corner cases etc > > >> wrt the freeze etc than I was with the larger fix proposed earlier. > > > I am bit lost. I thought Andrew NACKed this? > > > > I didn't, as I am not in a position to. I have not tested the result, > > but believe it is sufficient to fix the exact error at hand. If the > > maintainers think it is the best solution then so be it, but I am still > > of the opinion that it is is still a hack upon a hack. > > At this point in a freeze I'm much happier with: > > tools/pygrub/src/pygrub | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) The same here. This is now the season of handing out band-aids. As such Release-Acked-by: Konrad Rzeszutek Wilk > > than > 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(-) > > Plus Boris' patch is far easier to reason about in isolation in a > dynamically/duck typed language. > > Ian. >