Re: [Xen-devel] [OSSTEST Nested PATCH 1/6] parsing grub which has 'submenu' primitive

["Pang, LongtaoX" <longtaox.pang@xxxxxxxxx>]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@xxxxxxxxxx]
> Sent: Friday, March 20, 2015 12:17 AM
> To: Pang, LongtaoX
> Cc: xen-devel@xxxxxxxxxxxxx; Ian.Jackson@xxxxxxxxxxxxx; wei.liu2@xxxxxxxxxx;
> Hu, Robert
> Subject: Re: [OSSTEST Nested PATCH 1/6] parsing grub which has 'submenu'
> primitive
> 
> On Tue, 2015-03-17 at 14:16 -0400, longtao.pang wrote:
> > From: "longtao.pang" <longtaox.pang@xxxxxxxxx>
> >
> > 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 will add it next time.
> 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.
I will keep the original coding format next time.
> 
> >                  $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;
> >              }
> >          }
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel