[OSSTEST Nested PATCH 0/6] Introduction of netsted HVM test job

[OSSTEST Nested PATCH 0/6] Introduction of netsted HVM test job

[longtao.pang]

This patch set adds nested HVM test case for osstest.
In this test case, a Xen hypervisor (L1) runs on top of another Xen hypervisor (L0). 
Upon L1 hypervisor, we will then create a nested guest (L2), and test if the Linux guest can then be installed and run well.
About nested Xen virtualization, refer to http://wiki.xenproject.org/wiki/Nested_Virtualization_in_Xen.

Test steps
	0. To run osstest in standalone mode, write a config file in
           '~/.xen-osstest/config', and then create a standalone.config file to
           define 'TREE_LINUX', 'REVISION_LINUX' and 'NESTED_OS_IMAGE' which
           will be used for nested test. The directry path of 'Debian Images'
           could be difined in '~/.xen-osstest/config'. 
	1. run 'build-amd64' job and then 'build-amd64-pvops', to prepare xen
           installation tarball and hvm guest kernel.
	2. run 'test-amd64-amd6-nested' job, it does following:
		a. invoke test step of 'ts-debain-hvm-install' to install 
		   a normal HVM guest
		b. invoke test step of 'ts-nested-setup' to make some
                   appropriate runvars which selecthost would recognise and
                   prepare the configurations for installing L2 guest VM. 
		c. invoke test step of 'ts-xen-install' to install xen on 
		   the normal guest, alter it into a L1 hypervisor
		d. invoke test step of 'ts-debain-hvm-install' again, but 
	           take the L1 hypervisor as host, install the L2 guest on it
		e. invoke test step of 'ts-guest-stop', stop L2 guest.
                f. invoke test step of 'ts-guest-destroy' to destroy L1 guest.

This patch set reuses 'ts-debian-hvm-install' for both L1 installation and L2 installation, define 'nested' and 'nested2' as L1 and L2's hostname, define 'nested_l1 as L1's host ident.
It also reuses 'ts-xen-install' with 'nested' input param to differentiate L1 Xen installation from L0 Xen installation.
This patch series has been tested on test machines of amd64 arch, debian-7.6.0-amd64 as guests OS, with hvm domain0 of Linux kernel 3.18.5, in standalone mode.
----------------------------------------------------------------
longtao.pang (6):
      parsing grub which has 'submenu' primitive
      Add and expose some testsupport APIs
      Changes on test step of debain hvm guest install in hvm.
      Add new ts-nested-setup script to custmize nested test configuration before     the testing.
      Add test job for nest test case
      Compose the main recipe of test-nested test job.

 Osstest/Debian.pm      |   52 ++++++++++++++++++++++++++++++++--------------------
 Osstest/TestSupport.pm |   21 +++++++++++++++++++--
 make-flight            |   20 ++++++++++++++++++++
 sg-run-job             |   11 +++++++++++
 ts-debian-hvm-install  |   14 +++++++++++---
 ts-nested-setup        |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 147 insertions(+), 25 deletions(-)
 create mode 100755 ts-nested-setup

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

[longtao.pang]

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.
---
 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;
+                }
                 $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;
             }
         }
-- 
1.7.10.4

[OSSTEST Nested PATCH 2/6] Add and expose some testsupport APIs

[longtao.pang]

From: "longtao.pang" <longtaox.pang@intel.com>

1. Designate vif model to 'e1000', otherwise, with default device model,
the L1 eth0 interface disappear, hence xenbridge cannot work.
Maybe this limitation can be removed later after some fix it. For now, we
have to accomodate to it.
2. Since reboot L1 guest VM will take more time to boot up, we increase
multi-times for reboot-confirm-booted if test nested job, and the multi value 
is stored as a runvar in 'ts-nested-setup' script. Added another function 
'guest_editconfig_cd' and expose it, this function bascically changes guest
boot device sequence, alter its on_reboot behavior to restart and enabled 
nestedhvm feature.
3. In L2 installation context, its host (L1) IP address is not queried from DNS,
but after running "ts-nested-setup + host + nested", L1 IP is stored in runvar.
---
 Osstest/TestSupport.pm |   21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index e1ebcb7..57c7167 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -111,6 +111,7 @@ BEGIN {
                       iso_gen_flags_basic
                       iso_copy_content_from_image
                       guest_editconfig_nocd
+                      guest_editconfig_cd
                       );
     %EXPORT_TAGS = ( );
 
@@ -819,6 +820,10 @@ sub selecthost ($) {
 	logm("Host $name is in HostGroup $ho->{Properties}{HostGroup}");
     }
 
+    if ( $r{"${name}_ip"} ) {
+        $setprop->("IpAddr", $r{"${name}_ip"});
+    }
+
     # Next, we use the config file's general properites as defaults
     foreach my $k (keys %c) {
 	next unless $k =~ m/^HostProp_([A-Z].*)$/;
@@ -990,8 +995,9 @@ sub common_toolstack ($) {
 
 sub host_reboot ($) {
     my ($ho) = @_;
+    my $wait_time = $r{multi_reboot_time} ? $r{multi_reboot_time}*40 : 40;
     target_reboot($ho);
-    poll_loop(40,2, 'reboot-confirm-booted', sub {
+    poll_loop($wait_time,2, 'reboot-confirm-booted', sub {
         my $output;
         if (!eval {
             $output= target_cmd_output($ho, <<END, 40);
@@ -1541,7 +1547,7 @@ sub prepareguest_part_xencfg ($$$$$) {
     my $xencfg= <<END;
 name        = '$gho->{Name}'
 memory = ${ram_mb}
-vif         = [ 'type=ioemu,mac=$gho->{Ether}' ]
+vif         = [ 'type=ioemu,model=e1000,mac=$gho->{Ether}' ]
 #
 on_poweroff = '$onpoweroff'
 on_reboot   = '$onreboot'
@@ -2113,4 +2119,15 @@ sub guest_editconfig_nocd ($$) {
     });
 }
 
+sub guest_editconfig_cd ($) {
+    my ($gho) = @_;
+    guest_editconfig($gho->{Host}, $gho, sub {
+        if (m/^\s*boot\s*= '\s*d\s*c\s*'/) {
+            s/dc/cd/;
+        }
+        s/^on_reboot.*/on_reboot='restart'/;
+        s/#nestedhvm/nestedhvm/;
+    });
+}
+
 1;
-- 
1.7.10.4

[OSSTEST Nested PATCH 3/6] Changes on test step of debain hvm guest install in hvm.

[longtao.pang]

From: "longtao.pang" <longtaox.pang@intel.com>

1. Increase disk size to accomodate to nested test requirment.
2. Since 'Debain-xxx-.iso' image will be stored there, therefore needs more
disk capacity, increase root partition size in preseed generation.
3. In L1 installation context, assign more memory to it; since it
acts as a nested hypervisor anyway.
4. In hvm guest configuration file, add '#nestedhvm=1',
which will later be uncommented by guest_editconfig_cd() after
xen installed in L1, and about to boot into a nested xen environment.
---
 ts-debian-hvm-install |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
index 449b96c..f334e30 100755
--- a/ts-debian-hvm-install
+++ b/ts-debian-hvm-install
@@ -36,7 +36,7 @@ our $ho= selecthost($whhost);
 
 # guest memory size will be set based on host free memory, see below
 our $ram_mb;
-our $disk_mb= 10000;
+our $disk_mb= 15000;
 
 our $guesthost= "$gn.guest.osstest";
 our $gho;
@@ -60,7 +60,7 @@ d-i partman-auto/expert_recipe string \\
                         use_filesystem{ } filesystem{ vfat } \\
                         mountpoint{ /boot/efi } \\
                 . \\
-                5000 50 5000 ext4 \\
+                10000 50 10000 ext4 \\
                         method{ format } format{ } \\
                         use_filesystem{ } filesystem{ ext4 } \\
                         mountpoint{ / } \\
@@ -152,6 +152,7 @@ sub prep () {
     more_prepareguest_hvm($ho,$gho, $ram_mb, $disk_mb,
                           OnReboot => 'preserve',
                           Bios => $r{bios},
+                          ExtraConfig => '#nestedhvm=1',
                           PostImageHook => sub {
         my $cmds = iso_copy_content_from_image($gho, $newiso);
         $cmds .= prepare_initrd($initrddir,$newiso,$preseed_file_path);
@@ -173,6 +174,8 @@ my $ram_minslop = 100;
 my $ram_lots = 5000;
 if ($host_freemem_mb > $ram_lots * 2 + $ram_minslop) {
     $ram_mb = $ram_lots;
+} elsif ($gn eq 'nested') {
+    $ram_mb = 3072;
 } else {
     $ram_mb = 768;
 }
@@ -189,7 +192,12 @@ if ($stage<2) {
     guest_destroy($gho);
 }
 
-guest_editconfig_nocd($gho,$emptyiso);
+if ($gn eq 'nested') {
+    guest_editconfig_cd($gho);
+} else {
+    guest_editconfig_nocd($gho,$emptyiso);
+}
+
 guest_create($gho);
 guest_await_dhcp_tcp($gho,300);
 guest_check_up($gho);
-- 
1.7.10.4

[OSSTEST Nested PATCH 4/6] Add new ts-nested-setup script to custmize nested test configuration before the testing.

[longtao.pang]

From: "longtao.pang" <longtaox.pang@intel.com>

1. In this script, make some appropriate runvars which selecthost would recognise.
2. Prepare the configurations for installing L2 guest VM.
3. Create a lv disk in L0 and hot-attach it to L1, need to restart L1 to make
the block disk to be recognized by L1 system, then using this disk to create a VG that
used for installing L2.
---
 ts-nested-setup |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100755 ts-nested-setup

diff --git a/ts-nested-setup b/ts-nested-setup
new file mode 100755
index 0000000..69dc407
--- /dev/null
+++ b/ts-nested-setup
@@ -0,0 +1,54 @@
+#!/usr/bin/perl -w
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (C) 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
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+# 
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+use strict qw(vars);
+use DBI;
+use Osstest;
+use Osstest::Debian;
+use Osstest::TestSupport;
+
+tsreadconfig();
+our ($ident_l0,$nested_host) = @ARGV;
+our ($l0,$l1) = ts_get_host_guest(@ARGV);
+
+guest_check_ip($l1);
+target_cmd_root($l1, "mkdir -p /home/osstest/.ssh && cp /root/.ssh/authorized_keys /home/osstest/.ssh/");
+my $url = $c{WebspaceUrl}.$c{WebspaceCommon}.$l0->{Name}."_".'overlay.tar'; 
+target_cmd_root($l1, <<END);
+    wget -O overlay.tar $url
+    tar -xf overlay.tar -C /
+    rm overlay.tar -f
+    update-rc.d osstest-confirm-booted start 99 2 .
+END
+
+store_runvar('nested_l1',$nested_host);
+store_runvar("${nested_host}_ip",$l1->{Ip});
+store_runvar('multi_reboot_time',5);
+
+my $l2_disk_mb = 20000;
+my $l2_lv_name = 'nestedl2';
+my $vgname = $l0->{Name};
+$vgname .= ".$c{TestHostDomain}" if ($l0->{Suite} =~ m/lenny/);
+target_cmd_root($l0, <<END);
+    lvremove -f /dev/${vgname}/${l2_lv_name} ||:
+    lvcreate -L ${l2_disk_mb}M -n $l2_lv_name $vgname
+    dd if=/dev/zero of=/dev/${vgname}/${l2_lv_name} count=10
+    xl block-attach $l1->{Name} /dev/${vgname}/${l2_lv_name},raw,sdb,rw
+END
+target_install_packages_norec($l1, qw(lvm2 rsync genisoimage));
+target_reboot($l1);
+target_cmd_root($l1, "pvcreate /dev/xvdb && vgcreate ${l2_lv_name}_vg /dev/xvdb");
-- 
1.7.10.4

[OSSTEST Nested PATCH 5/6] Add test job for nest test case

[longtao.pang]

From: "longtao.pang" <longtaox.pang@intel.com>

This patch adds creation of the nested test job, when job creation procedure
is invoked.
'NESTED_OS_IMAGE' is the name of 'Debian ISO Images', which defined in
 standalone.config.
---
 make-flight |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/make-flight b/make-flight
index 63b14f5..fb35ac4 100755
--- a/make-flight
+++ b/make-flight
@@ -204,6 +204,25 @@ do_hvm_win7_x64_tests () {
             all_hostflags=$most_hostflags,hvm
 }
 
+do_hvm_debian_nested_tests () {
+  if [ $xenarch != amd64 ]; then
+    return
+  fi
+  if [ $dom0arch != amd64 ]; then
+    return
+  fi
+
+  job_create_test test-$xenarch$kern-$dom0arch-nested test-nested xl \
+			$xenarch $dom0arch \
+            nested_image=$NESTED_OS_IMAGE \
+            nested2_image=$NESTED_OS_IMAGE \
+            bios=seabios \
+            kernbuildjob=build-amd64-pvops \
+            kernkind=pvops \
+            device_model_version=qemu-xen \
+            all_hostflags=$most_hostflags,hvm
+}
+
 do_hvm_debian_test_one () {
   testname=$1
   bios=$2
@@ -401,6 +420,7 @@ test_matrix_do_one () {
     done
 
   fi
+  do_hvm_debian_nested_tests
   do_passthrough_tests
 }
 
-- 
1.7.10.4

[OSSTEST Nested PATCH 6/6] Compose the main recipe of test-nested test job.

[longtao.pang]

From: "longtao.pang" <longtaox.pang@intel.com>

---
 sg-run-job |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sg-run-job b/sg-run-job
index 94d091b..ababebe 100755
--- a/sg-run-job
+++ b/sg-run-job
@@ -297,6 +297,17 @@ proc run-job/test-pair {} {
 #    run-ts . remus-failover ts-remus-check         src_host dst_host + debian
 }
 
+proc need-hosts/test-nested {} {return host}
+proc run-job/test-nested {} {
+    run-ts . = ts-debian-hvm-install + host + nested
+    run-ts . = ts-nested-setup + host + nested
+    run-ts . = ts-xen-install nested_l1
+    run-ts . = ts-host-reboot nested_l1
+    run-ts . = ts-debian-hvm-install nested_l1 nested2
+    run-ts . = ts-guest-stop nested_l1 nested2
+    run-ts . = ts-guest-destroy host nested
+}
+
 proc test-guest-migr {g} {
     if {[catch { run-ts . = ts-migrate-support-check + host $g }]} return
 
-- 
1.7.10.4

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

[Ian Campbell]

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;
>              }
>          }

Re: [OSSTEST Nested PATCH 2/6] Add and expose some testsupport APIs

[Ian Campbell]

On Tue, 2015-03-17 at 14:16 -0400, longtao.pang wrote:
> From: "longtao.pang" <longtaox.pang@intel.com>
> 
> 1. Designate vif model to 'e1000', otherwise, with default device model,
> the L1 eth0 interface disappear, hence xenbridge cannot work.
> Maybe this limitation can be removed later after some fix it. For now, we
> have to accomodate to it.

You have done this unconditionally, which means it affects all guests.
You need to make this configurable by the caller, probably by plumbing
it through in $xopts (a hash of extra options).

I see now you were told this last time around by Ian J, please don't
just resend such things without change either fix them, make an argument
for doing it your way or ask for clarification if you don't understand
the requested change.

> 2. Since reboot L1 guest VM will take more time to boot up, we increase
> multi-times for reboot-confirm-booted if test nested job, and the multi value 
> is stored as a runvar in 'ts-nested-setup' script. Added another function 
> 'guest_editconfig_cd' and expose it, this function bascically changes guest
> boot device sequence, alter its on_reboot behavior to restart and enabled 
> nestedhvm feature.

This looks like two items run together?

The multi_reboot_time thing sounds ok, but it should be called
reboot_time_factor or something like that. In fact I see that Ian
suggested previously that it should have the host ident in it, that
makes sense to me.

The editconfig_cd thing -- yet another thing which Ian questioned and
which it was agreed you would change but you haven't.

I think perhaps you have accidentally resent an older version of the
series. If not then please go back and ensure you have addressed all of
the feedback given on the last iteration before sending another version.

Ian.

Re: [OSSTEST Nested PATCH 0/6] Introduction of netsted HVM test job

[Ian Campbell]

On Tue, 2015-03-17 at 14:16 -0400, longtao.pang wrote:
> This patch set adds nested HVM test case for osstest.

I've now looked at the first two patches in this series and I've found
that in both patches you have consistently not reacted to the review
comments made the first time around, so I'm not going to read the rest
of the series now since it would appear to be a waste of my time if I'm
just going to end up repeating things which were said last time.

Please resend a 3rd version when you have made sure that you have
addressed the previous feedback.

Please also:

      * Use "git send-email --reroll-count=N" (or with older git
        --subject-prefix including vN) to indicate which revision of the
        patch series this is (i.e. here you should have used
        --reroll-count=2 and next time 3)
      * Add a miniture changelog to each patch indication what has
        changed in this iteration, this should go after the commit
        message, S-o-b and a "---" marker.

See the "[PATCH v2] foobar: Add a new trondle calls" example in
http://wiki.xen.org/wiki/Submitting_Xen_Patches#Review.2C_Rinse_.26_Repeat for example of both of these.

In fact please read wiki.xen.org/wiki/Submitting_Xen_Patches for lots of
hints on all of this stuff.

Ian.

Re: [OSSTEST Nested PATCH 0/6] Introduction of netsted HVM test job

[Pang, LongtaoX]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: Friday, March 20, 2015 12:32 AM
> To: Pang, LongtaoX
> Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com; wei.liu2@citrix.com;
> Hu, Robert
> Subject: Re: [OSSTEST Nested PATCH 0/6] Introduction of netsted HVM test job
> 
> On Tue, 2015-03-17 at 14:16 -0400, longtao.pang wrote:
> > This patch set adds nested HVM test case for osstest.
> 
> I've now looked at the first two patches in this series and I've found that in both
> patches you have consistently not reacted to the review comments made the
> first time around, so I'm not going to read the rest of the series now since it
> would appear to be a waste of my time if I'm just going to end up repeating
> things which were said last time.
> 
I am sorry that this version still does not reach the requirement and take your time to review it.
Actually, this version of patches change a lot according Ian Jackson's last useful comments.
> Please resend a 3rd version when you have made sure that you have addressed
> the previous feedback.
> 
Actually, this is the sixth version, the next version will be seventh, I will mark the version number of patches next time.
> Please also:
> 
>       * Use "git send-email --reroll-count=N" (or with older git
>         --subject-prefix including vN) to indicate which revision of the
>         patch series this is (i.e. here you should have used
>         --reroll-count=2 and next time 3)
>       * Add a miniture changelog to each patch indication what has
>         changed in this iteration, this should go after the commit
>         message, S-o-b and a "---" marker.
> 
> See the "[PATCH v2] foobar: Add a new trondle calls" example in
> http://wiki.xen.org/wiki/Submitting_Xen_Patches#Review.2C_Rinse_.26_Repe
> at for example of both of these.
> 
> In fact please read wiki.xen.org/wiki/Submitting_Xen_Patches for lots of hints
> on all of this stuff.
> 
This suggestion is really very useful to me, because I am not familiar with that.
> Ian.
> 

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

[Pang, LongtaoX]

> -----Original Message-----
> From: Ian Campbell [mailto:ian.campbell@citrix.com]
> Sent: Friday, March 20, 2015 12:17 AM
> To: Pang, LongtaoX
> Cc: xen-devel@lists.xen.org; Ian.Jackson@eu.citrix.com; wei.liu2@citrix.com;
> 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@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 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;
> >              }
> >          }
>