From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [OSSTEST Nested PATCH 1/6] parsing grub which has 'submenu' primitive Date: Thu, 19 Mar 2015 16:17:00 +0000 Message-ID: <1426781820.21742.53.camel@citrix.com> References: <1426616214-11670-1-git-send-email-longtaox.pang@intel.com> <1426616214-11670-2-git-send-email-longtaox.pang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1426616214-11670-2-git-send-email-longtaox.pang@intel.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: "longtao.pang" Cc: wei.liu2@citrix.com, robert.hu@intel.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On Tue, 2015-03-17 at 14:16 -0400, longtao.pang wrote: > From: "longtao.pang" > > From a hvm kernel build from Linux stable Kernel tree, > the auto generated grub2 menu will have 'submenu' primitive, upon the > 'menuentry' items. Xen boot entries will be grouped into a submenu. This > patch adds capability to support such grub formats. This is missing a Signed-off-by per http://wiki.xen.org/wiki/Submitting_Xen_Patches#Signing_off_a_patch I think this also means that the current patching of the grub.cfg which Wei added for the host level stuff can be reverted. > --- > Osstest/Debian.pm | 52 ++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 32 insertions(+), 20 deletions(-) > > diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm > index e3e1c90..9fdacd7 100644 > --- a/Osstest/Debian.pm > +++ b/Osstest/Debian.pm > @@ -1,5 +1,6 @@ > # This is part of "osstest", an automated testing framework for Xen. > # Copyright (C) 2009-2013 Citrix Inc. > +# Copyright (C) 2014-2015 Intel Inc. > # > # This program is free software: you can redistribute it and/or modify > # it under the terms of the GNU Affero General Public License as published by > @@ -362,26 +363,34 @@ sub setupboot_grub2 ($$$) { > > my $count= 0; > my $entry; > + my $submenu; > while (<$f>) { > next if m/^\s*\#/ || !m/\S/; > if (m/^\s*\}\s*$/) { > - die unless $entry; > + die unless $entry || $submenu; > + if(!defined $entry && defined $submenu){ > + logm("Met end of a submenu starting from ". > + "$submenu->{StartLine}. ". > + "Our want kern is $want_kernver"); > + $submenu=undef; > + next; > + } > my (@missing) = > - grep { !defined $entry->{$_} } > - (defined $xenhopt > - ? qw(Title Hv KernDom0 KernVer) > - : qw(Title Hv KernOnly KernVer)); > - if (@missing) { > - logm("(skipping entry at $entry->{StartLine};". > - " no @missing)"); > - } elsif (defined $want_kernver && > - $entry->{KernVer} ne $want_kernver) { > - logm("(skipping entry at $entry->{StartLine};". > - " kernel $entry->{KernVer}, not $want_kernver)"); > - } else { > - # yes! > - last; > - } > + grep { !defined $entry->{$_} } > + (defined $xenhopt > + ? qw(Title Hv KernDom0 KernVer) > + : qw(Title Hv KernOnly KernVer)); > + if (@missing) { > + logm("(skipping entry at $entry->{StartLine};". > + " no @missing)"); > + } elsif (defined $want_kernver && > + $entry->{KernVer} ne $want_kernver) { > + logm("(skipping entry at $entry->{StartLine};". > + " kernel $entry->{KernVer}, not $want_kernver)"); > + } else { > + # yes! > + last; > + } Please avoid unnecessary reindentation or code reformatting mixed with functional changes. Otherwise you just obscure the actual changes you are making. If something is wrongly indented or formatted then please fix in another patch first. As it is I'm afraid this patch is basically unreviewable. > $entry= undef; > next; > } > @@ -393,21 +402,24 @@ sub setupboot_grub2 ($$$) { > $entry= { Title => $1, StartLine => $., Number => $count }; > $count++; > } > - if (m/^\s*multiboot\s*\/(xen\-[0-9][-+.0-9a-z]*\S+)/) { > + if(m/^submenu\s+[\'\"](.*)[\'\"].*\{\s*$/){ > + $submenu={ StartLine =>$.}; > + } > + if (m/^\s*multiboot\s*(?:\/boot)*\/(xen\S+)/) { > die unless $entry; > $entry->{Hv}= $1; > } > - if (m/^\s*multiboot\s*\/(vmlinu[xz]-(\S+))/) { > + if (m/^\s*multiboot\s*(?:\/boot)*\/(vmlinu[xz]-(\S+))/) { > die unless $entry; > $entry->{KernOnly}= $1; > $entry->{KernVer}= $2; > } > - if (m/^\s*module\s*\/(vmlinu[xz]-(\S+))/) { > + if (m/^\s*module\s*(?:\/boot)*\/(vmlinu[xz]-(\S+))/) { > die unless $entry; > $entry->{KernDom0}= $1; > $entry->{KernVer}= $2; > } > - if (m/^\s*module\s*\/(initrd\S+)/) { > + if (m/^\s*module\s*(?:\/boot)*\/(initrd\S+)/) { > $entry->{Initrd}= $1; > } > }