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

[Ian Campbell <ian.campbell@citrix.com>]

On Tue, 2015-03-17 at 14:16 -0400, longtao.pang wrote:
> From: "longtao.pang" <longtaox.pang@intel.com>
> 
> 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;
>              }
>          }