[PATCH v2 OSSTEST 0/18] Implement for driving libvirt via virsh

[PATCH v2 OSSTEST 0/18] Implement for driving libvirt via virsh

[Ian Campbell]

The following series switches osstest to implement the toolstack via
get_host_method_object()->method rather than toolstack()->{Command}."
method" etc.

This is needed because virsh differs from xm/xl in a few commands.

It also implements partial virsh support (simple lifecycle stuff, but
not e.g. migration yet). Due to the ts-migration-check logic this means
that the libvirt sequence works in so far as it skips/ignores the
migration/save+restore related tests.

I've not kept notes on what I changes since last time (but noone has
really reviewed the v1/RFC anyway ;-)). This time around is more
complete in that it implements shutdown etc and the job completes and
I've actually removed ->{Command} which was just an aspiration last
timearound.

I've tested with:

test-amd64-amd64-libvirt on a xen-unstable flight, which failed due to
http://article.gmane.org/gmane.comp.emulators.xen.devel/224197 and that
set of issues.

test-amd64-amd64-libvirt on a xen-unstable flight with the above patches
added, works.

Since this touches xend code paths I've also tested using a
xen-4.4-testing based flight:

test-amd64-amd64-libvirt
test-amd64-amd64-pv (uses xend)
test-amd64-amd64-xl (uses xl, duh!)

All passed.

The last patch stubs out migration a bit but doesn't actually work and
obviously shouldn't be applied, it's included for reference. Note that
the series still enables useful functionality even without migration
(e.g. start/stop/destroy testing)

Ian.

[PATCH OSSTEST v2 01/18] apt: lock osstest's usages of apt-get against each other

[Ian Campbell]

Currently we rely on all apt-get invocations being in a single
ts-xen-build-prep job which can't run on a shared host.

That is a bit inflexible so instead use our own lock. We wait indefinitely and
rely on osstest's existing command timeout infrastructure to catch problems.

target_install_packages*() previous estimated the time taken to install the
packages based on the number of packages. This no longer applies because the
install might get stuck behind some other large install. Use a 3000s (nearly
an hour) timeout instead (I expect failures here to be unusual so erred on the
big side)

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Debian.pm      | 2 +-
 Osstest/TestSupport.pm | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Osstest/Debian.pm b/Osstest/Debian.pm
index c8db601..b343f0f 100644
--- a/Osstest/Debian.pm
+++ b/Osstest/Debian.pm
@@ -492,7 +492,7 @@ d-i apt-setup/another boolean false
 d-i apt-setup/non-free boolean false
 d-i apt-setup/contrib boolean false
 
-d-i pkgsel/include string openssh-server, ntp, ntpdate, ethtool, $extra_packages
+d-i pkgsel/include string openssh-server, ntp, ntpdate, ethtool, chiark-utils-bin, $extra_packages
 
 $xopts{ExtraPreseed}
 
diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index e6c54bc..2ac490f 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -433,17 +433,18 @@ sub target_putfile_root ($$$$;$) {
 sub target_run_apt {
     my ($ho, $timeout, @aptopts) = @_;
     target_cmd_root($ho,
-   "DEBIAN_PRIORITY=critical UCF_FORCE_CONFFOLD=y apt-get @aptopts",
+   "DEBIAN_PRIORITY=critical UCF_FORCE_CONFFOLD=y \
+        with-lock-ex -w /var/lock/osstest-apt apt-get @aptopts",
                     $timeout);
 }
 sub target_install_packages {
     my ($ho, @packages) = @_;
-    target_run_apt($ho, 300 + 100 * @packages,
+    target_run_apt($ho, 3000,
 		   qw(-y install), @packages);
 }
 sub target_install_packages_norec {
     my ($ho, @packages) = @_;
-    target_run_apt($ho, 300 + 100 * @packages,
+    target_run_apt($ho, 3000,
 		   qw(--no-install-recommends -y install), @packages);
 }
 
-- 
2.1.1

Re: [PATCH OSSTEST v2 01/18] apt: lock osstest's usages of apt-get against each other

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 01/18] apt: lock osstest's usages of apt-get against each other"):
> Currently we rely on all apt-get invocations being in a single
> ts-xen-build-prep job which can't run on a shared host.
> 
> That is a bit inflexible so instead use our own lock. We wait
> indefinitely and rely on osstest's existing command timeout
> infrastructure to catch problems.

...
> diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
> index e6c54bc..2ac490f 100644
> --- a/Osstest/TestSupport.pm
> +++ b/Osstest/TestSupport.pm
> @@ -433,17 +433,18 @@ sub target_putfile_root ($$$$;$) {
>  sub target_run_apt {
>      my ($ho, $timeout, @aptopts) = @_;
>      target_cmd_root($ho,
> -   "DEBIAN_PRIORITY=critical UCF_FORCE_CONFFOLD=y apt-get @aptopts",
> +   "DEBIAN_PRIORITY=critical UCF_FORCE_CONFFOLD=y \
> +        with-lock-ex -w /var/lock/osstest-apt apt-get @aptopts",
>                      $timeout);

I think target_run_apt ought to lose the $timeout parameter.  Anyone
who calls it with other than the big 3000s timeout might lose, after
all.

Ian.

Re: [PATCH OSSTEST v2 01/18] apt: lock osstest's usages of apt-get against each other

[Ian Campbell]

On Tue, 2015-01-20 at 18:19 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST v2 01/18] apt: lock osstest's usages of apt-get against each other"):
> > Currently we rely on all apt-get invocations being in a single
> > ts-xen-build-prep job which can't run on a shared host.
> > 
> > That is a bit inflexible so instead use our own lock. We wait
> > indefinitely and rely on osstest's existing command timeout
> > infrastructure to catch problems.
> 
> ...
> > diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
> > index e6c54bc..2ac490f 100644
> > --- a/Osstest/TestSupport.pm
> > +++ b/Osstest/TestSupport.pm
> > @@ -433,17 +433,18 @@ sub target_putfile_root ($$$$;$) {
> >  sub target_run_apt {
> >      my ($ho, $timeout, @aptopts) = @_;
> >      target_cmd_root($ho,
> > -   "DEBIAN_PRIORITY=critical UCF_FORCE_CONFFOLD=y apt-get @aptopts",
> > +   "DEBIAN_PRIORITY=critical UCF_FORCE_CONFFOLD=y \
> > +        with-lock-ex -w /var/lock/osstest-apt apt-get @aptopts",
> >                      $timeout);
> 
> I think target_run_apt ought to lose the $timeout parameter.  Anyone
> who calls it with other than the big 3000s timeout might lose, after
> all.

Good point.

FWIW I don't think this patch is actually necessary for this series, it
arose during an early version because I was installing stuff in
ts-build-libvirt, but now everything is done in e.g. ts-build-prep.

I think it's still a step in the direction you wanted wrt sharing
machine though, so I'll keep it in the series next time around too.

Ian.

Re: [PATCH OSSTEST v2 01/18] apt: lock osstest's usages of apt-get against each other

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH OSSTEST v2 01/18] apt: lock osstest's usages of apt-get against each other"):
> On Tue, 2015-01-20 at 18:19 +0000, Ian Jackson wrote:
> > I think target_run_apt ought to lose the $timeout parameter.  Anyone
> > who calls it with other than the big 3000s timeout might lose, after
> > all.
> 
> Good point.
> 
> FWIW I don't think this patch is actually necessary for this series, it
> arose during an early version because I was installing stuff in
> ts-build-libvirt, but now everything is done in e.g. ts-build-prep.

Ah.

> I think it's still a step in the direction you wanted wrt sharing
> machine though, so I'll keep it in the series next time around too.

Yes, please.

Thanks,
Ian.

[PATCH OSSTEST v2 02/18] ts-logs-capture: Collect some libvirt logs and capabilities

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 ts-logs-capture | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/ts-logs-capture b/ts-logs-capture
index 21974a9..6cf51c1 100755
--- a/ts-logs-capture
+++ b/ts-logs-capture
@@ -117,6 +117,9 @@ sub fetch_logs_host_guests () {
                   /var/log/xen/osstest*
                   /var/log/xen/xenstored*
 
+                  /var/log/libvirt/libvirtd.log
+                  /var/log/libvirt/libxl/*
+
                   /var/run/xenstored*
                   /var/log/xenstored*
 
@@ -161,6 +164,7 @@ sub fetch_logs_host_guests () {
          'lspci -vvv',
          'lspci -tv',
          'cat /proc/partitions',
+         'virsh capabilities',
          ) {
             try_cmd_output_save($cmd);
         }
-- 
2.1.1

[PATCH OSSTEST v2 02/18] ts-logs-capture: Collect some libvirt logs and capabilities

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 02/18] ts-logs-capture: Collect some libvirt logs and capabilities"):
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH OSSTEST v2 03/18] Pass host to toolstack()

[Ian Campbell]

This will be needed in a future patch.

Everywhere already has a $ho in hand. Also cache the answer as
$ho->{Toolstack}.

I scanned the source with:
    find -name \*.pm -exec perl -c {} \;
    for i in ts-* ; do perl -c $i; done
which reported "Not enough arguments for Osstest::TestSupport::toolstack" for
each callsite which needed changing.

Also don't pass the toolstack command name directly to host_get_free_memory().
Look it up instead.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/TestSupport.pm         | 16 ++++++++++------
 ts-debian-fixup                |  2 +-
 ts-debian-hvm-install          |  4 ++--
 ts-guest-localmigrate          |  2 +-
 ts-guest-migrate               |  2 +-
 ts-guest-saverestore           |  8 ++++----
 ts-guest-start                 |  4 ++--
 ts-guest-stop                  |  2 +-
 ts-logs-capture                |  2 +-
 ts-migrate-support-check       |  4 ++--
 ts-redhat-install              |  2 +-
 ts-rumpuserxen-demo-xenstorels |  4 ++--
 ts-windows-install             |  2 +-
 ts-xen-install                 | 10 +++++-----
 14 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 2ac490f..b7887bc 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -954,8 +954,9 @@ END
     });
 }
 
-sub host_get_free_memory($$) {
-    my ($ho,$toolstack) = @_;
+sub host_get_free_memory($) {
+    my ($ho) = @_;
+    my $toolstack = toolstack($ho)->{Command};
     # The line is as followed:
     # free_memory       :   XXXX
     my $info = target_cmd_output_root($ho, "$toolstack info", 10);
@@ -1318,7 +1319,7 @@ sub guest_await_shutdown ($$$) {
 
 sub guest_destroy ($$) {
     my ($ho,$gho) = @_;
-    target_cmd_root($ho, toolstack()->{Command}." destroy $gho->{Name}", 40);
+    target_cmd_root($ho, toolstack($ho)->{Command}." destroy $gho->{Name}", 40);
 }
 
 sub guest_create ($$) {
@@ -1609,7 +1610,7 @@ sub guest_check_up ($) {
 
 sub guest_get_state ($$) {
     my ($ho,$gho) = @_;
-    my $domains= target_cmd_output_root($ho, toolstack()->{Command}." list");
+    my $domains= target_cmd_output_root($ho, toolstack($ho)->{Command}." list");
     $domains =~ s/^Name.*\n//;
     foreach my $l (split /\n/, $domains) {
         $l =~ m/^(\S+) (?: \s+ \d+ ){3} \s+ ([-a-z]+) \s/x or die "$l ?";
@@ -1814,7 +1815,7 @@ sub guest_find_domid ($$) {
     my ($ho,$gho) = @_;
     return if defined $gho->{Domid};
     my $list= target_cmd_output_root($ho,
-                toolstack()->{Command}." list $gho->{Name}");
+                toolstack($ho)->{Command}." list $gho->{Name}");
     $list =~ m/^(?!Name\s)(\S+)\s+(\d+)\s+(\d+)+(\d+)\s.*$/m
         or die "domain list: $list";
     $1 eq $gho->{Name} or die "domain list name $1 expected $gho->{Name}";
@@ -1884,7 +1885,9 @@ our %toolstacks=
         },
      );
 
-sub toolstack () {
+sub toolstack ($) {
+    my ($ho) = @_;
+    return $ho->{Toolstack} if $ho->{Toolstack};
     my $tsname= $r{toolstack};
     $tsname= 'xend' if !defined $tsname;
     my $ts= $toolstacks{$tsname};
@@ -1893,6 +1896,7 @@ sub toolstack () {
         logm("toolstack $tsname");
         $ts->{Name}= $tsname;
     }
+    $ho->{Toolstack} = $ts;
     return $ts;
 }
 
diff --git a/ts-debian-fixup b/ts-debian-fixup
index f001418..11e956c 100755
--- a/ts-debian-fixup
+++ b/ts-debian-fixup
@@ -142,7 +142,7 @@ sub otherfixupcfg () {
     if (@pcipt) {
         logm("checking passthrough device(s) are assignable: @pcipt");
         my @assignables= split /\n/,
-            target_cmd_output_root($ho, toolstack()->{Command}.
+            target_cmd_output_root($ho, toolstack($ho)->{Command}.
                                    " pci-assignable-list");
         foreach my $pcipt (@pcipt) {
             die "not assignable: $pcipt (not in: @assignables)"
diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
index 37eade2..0148eef 100755
--- a/ts-debian-hvm-install
+++ b/ts-debian-hvm-install
@@ -41,7 +41,7 @@ our $disk_mb= 10000;
 our $guesthost= "$gn.guest.osstest";
 our $gho;
 
-our $toolstack= toolstack()->{Command};
+our $toolstack= toolstack($ho)->{Command};
 
 
 sub preseed () {
@@ -171,7 +171,7 @@ sub prep () {
 
 # If host has >8G free memory, create a guest with 4G memory to catch
 # any error that triggers cross 4G boundary
-my $host_freemem_mb = host_get_free_memory($ho, $toolstack);
+my $host_freemem_mb = host_get_free_memory($ho);
 my $ram_minslop = 100;
 my $ram_lots = 5000;
 if ($host_freemem_mb > $ram_lots * 2 + $ram_minslop) {
diff --git a/ts-guest-localmigrate b/ts-guest-localmigrate
index e50e93a..f3381da 100755
--- a/ts-guest-localmigrate
+++ b/ts-guest-localmigrate
@@ -33,7 +33,7 @@ our ($ho,$gho) = ts_get_host_guest(@ARGV);
 sub migrate () {
     guest_checkrunning($ho,$gho) or die $gho->{Name};
     target_cmd_root($ho,
-		    toolstack()->{Command}
+		    toolstack($ho)->{Command}
 		    ." migrate $gho->{Name} localhost",
 		    $timeout{Migrate});
 }
diff --git a/ts-guest-migrate b/ts-guest-migrate
index 17ac8a0..65e7b42 100755
--- a/ts-guest-migrate
+++ b/ts-guest-migrate
@@ -32,7 +32,7 @@ sub migrate () {
     guest_checkrunning($sho,$gho) or die $gho->{Name};
     my $err= guest_check_ip($gho);  die "$err $gho->{Name}" if defined $err;
     target_cmd_root($sho,
-		    toolstack()->{Command}
+		    toolstack($sho)->{Command}
 		    ." migrate $gho->{Name} $dho->{Name}",
 		    $timeout{Migrate});
 }
diff --git a/ts-guest-saverestore b/ts-guest-saverestore
index 81671c8..9e04ae9 100755
--- a/ts-guest-saverestore
+++ b/ts-guest-saverestore
@@ -28,17 +28,17 @@ sub save () {
     guest_checkrunning($ho,$gho) or die $gho->{Name};
     my $err= guest_check_ip($gho);  die "$err $gho->{Name}" if defined $err;
     target_cmd_root($ho,
-		    toolstack()->{Command}
+		    toolstack($ho)->{Command}
 		    ." save $gho->{Name} image",
 		    200);
     target_ping_check_down($gho);
 }
 sub restore () {
     target_cmd_root($ho,
-		    toolstack()->{Command}
+		    toolstack($ho)->{Command}
 		    ." restore "
-		    .(toolstack()->{RestoreNeedsConfig} ?
-		      $r{ $gho->{Guest}.'_'. toolstack()->{CfgPathVar} } : '')
+		    .(toolstack($ho)->{RestoreNeedsConfig} ?
+		      $r{ $gho->{Guest}.'_'. toolstack($ho)->{CfgPathVar} } : '')
 		    ." image", 200);
     target_ping_check_up($gho);
 }
diff --git a/ts-guest-start b/ts-guest-start
index 057afe6..bfbb734 100755
--- a/ts-guest-start
+++ b/ts-guest-start
@@ -26,8 +26,8 @@ our ($ho,$gho) = ts_get_host_guest(@ARGV);
 
 sub start () {
     guest_umount_lv($ho, $gho);
-    my $cmd= toolstack()->{Command}." create ".
-        $r{ $gho->{Guest}.'_'. toolstack()->{CfgPathVar} };
+    my $cmd= toolstack($ho)->{Command}." create ".
+        $r{ $gho->{Guest}.'_'. toolstack($ho)->{CfgPathVar} };
     target_cmd_root($ho, $cmd, 30);
 }
 
diff --git a/ts-guest-stop b/ts-guest-stop
index cc7db4c..0e3a863 100755
--- a/ts-guest-stop
+++ b/ts-guest-stop
@@ -27,7 +27,7 @@ our ($ho,$gho) = ts_get_host_guest(@ARGV);
 sub stop () {
     guest_checkrunning($ho, $gho) or die "$gho->{Name} not running";
     target_cmd_root($ho,
-		    toolstack()->{Command}
+		    toolstack($ho)->{Command}
 		    ." shutdown -w "
 		    .$gho->{Name}, 200);
     guest_checkrunning($ho, $gho) and die $gho->{Name};
diff --git a/ts-logs-capture b/ts-logs-capture
index 6cf51c1..841ad5a 100755
--- a/ts-logs-capture
+++ b/ts-logs-capture
@@ -195,7 +195,7 @@ sub fetch_logs_guest ($) {
         logm("cannot find domid: $@");
         return;
     }
-    my $consolecmd= toolstack()->{Command}." console $gho->{Name}";
+    my $consolecmd= toolstack($ho)->{Command}." console $gho->{Name}";
     try_cmd_output_save("sleep 1 | $consolecmd | cat",
                         "guest-$gho->{Name}-console");
 
diff --git a/ts-migrate-support-check b/ts-migrate-support-check
index ffae1b3..c70b77a 100755
--- a/ts-migrate-support-check
+++ b/ts-migrate-support-check
@@ -25,9 +25,9 @@ tsreadconfig();
 our $ho = selecthost($ARGV[0]);
 
 # all xend/xm platforms support migration
-exit(0) if toolstack()->{Command} eq "xm";
+exit(0) if toolstack($ho)->{Command} eq "xm";
 
-my $help = target_cmd_output_root($ho, toolstack()->{Command}." help");
+my $help = target_cmd_output_root($ho, toolstack($ho)->{Command}." help");
 
 my $rc = ($help =~ m/^\s*migrate/m) ? 0 : 1;
 
diff --git a/ts-redhat-install b/ts-redhat-install
index 56d4129..a0b1fab 100755
--- a/ts-redhat-install
+++ b/ts-redhat-install
@@ -37,7 +37,7 @@ our $disk_mb= 50000;
 our $guesthost= "$gn.guest.osstest";
 our $gho;
 
-our $xl= toolstack()->{Command};
+our $xl= toolstack($ho)->{Command};
 
 
 sub kickstart () {
diff --git a/ts-rumpuserxen-demo-xenstorels b/ts-rumpuserxen-demo-xenstorels
index 6db7024..a2a6a77 100755
--- a/ts-rumpuserxen-demo-xenstorels
+++ b/ts-rumpuserxen-demo-xenstorels
@@ -40,8 +40,8 @@ sub arrangepreserve () {
 }
 
 sub start () {
-    my $cmd= toolstack()->{Command}." create ".
-        $r{ $gho->{Guest}.'_'. toolstack()->{CfgPathVar} };
+    my $cmd= toolstack($ho)->{Command}." create ".
+        $r{ $gho->{Guest}.'_'. toolstack($ho)->{CfgPathVar} };
     target_cmd_root($ho, $cmd, 30);
 
     $domid = target_cmd_output_root($ho,"xl domid $gho->{Guest}");
diff --git a/ts-windows-install b/ts-windows-install
index 0a69f8e..4b06310 100755
--- a/ts-windows-install
+++ b/ts-windows-install
@@ -50,7 +50,7 @@ END
 }
 
 sub start () {
-    target_cmd_root($ho, toolstack()->{Command}.
+    target_cmd_root($ho, toolstack($ho)->{Command}.
                     " create $gho->{CfgPath}", 100);
 }
 
diff --git a/ts-xen-install b/ts-xen-install
index 4d34d1f..7cfe344 100755
--- a/ts-xen-install
+++ b/ts-xen-install
@@ -60,8 +60,8 @@ sub packages () {
     if ($r{arch} eq 'i386') {
 	target_install_packages($ho, 'libc6-xen');
     }
-    target_install_packages($ho, @{toolstack()->{ExtraPackages}})
-        if toolstack()->{ExtraPackages};
+    target_install_packages($ho, @{toolstack($ho)->{ExtraPackages}})
+        if toolstack($ho)->{ExtraPackages};
 }
 
 sub extract () {
@@ -101,7 +101,7 @@ sub adjustconfig () {
 	    }
 	}
 	print EO $extra or die $!;
-    }) if toolstack()->{Name} eq "xend";
+    }) if toolstack($ho)->{Name} eq "xend";
 
     my $trace_config_file;
     foreach my $try (qw(/etc/default/xencommons
@@ -147,7 +147,7 @@ sub setupboot () {
     } else {
 	logm("No Xen console device defined for host");
     }
-    if (toolstack()->{Dom0MemFixed}) {
+    if (toolstack($ho)->{Dom0MemFixed}) {
         $xenhopt .= " dom0_mem=512M,max:512M";
     }
     my $append= $r{xen_boot_append};
@@ -179,7 +179,7 @@ sub setupboot () {
 our $initscripts_nobridge;
 
 sub setupinitd () {
-    my $ts= toolstack();
+    my $ts= toolstack($ho);
     my $xencommons= '/etc/init.d/xencommons';
     my $have_xencommons=
         !!target_cmd_output_root($ho, <<END);
-- 
2.1.1

Re: [PATCH OSSTEST v2 03/18] Pass host to toolstack()

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 03/18] Pass host to toolstack()"):
> This will be needed in a future patch.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH OSSTEST v2 04/18] ts-rumpuserxen-demo-xenstorels: Use standard functions for things

[Ian Campbell]

Specifically guest_create and guest_find_domid.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 ts-rumpuserxen-demo-xenstorels | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/ts-rumpuserxen-demo-xenstorels b/ts-rumpuserxen-demo-xenstorels
index a2a6a77..6698848 100755
--- a/ts-rumpuserxen-demo-xenstorels
+++ b/ts-rumpuserxen-demo-xenstorels
@@ -40,11 +40,9 @@ sub arrangepreserve () {
 }
 
 sub start () {
-    my $cmd= toolstack($ho)->{Command}." create ".
-        $r{ $gho->{Guest}.'_'. toolstack($ho)->{CfgPathVar} };
-    target_cmd_root($ho, $cmd, 30);
+    guest_create($gho,toolstack($ho));
 
-    $domid = target_cmd_output_root($ho,"xl domid $gho->{Guest}");
+    $domid = guest_find_domid($ho, $gho);
 }
 
 sub await_end () {
-- 
2.1.1

Re: [PATCH OSSTEST v2 04/18] ts-rumpuserxen-demo-xenstorels: Use standard functions for things

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 04/18] ts-rumpuserxen-demo-xenstorels: Use standard functions for things"):
> Specifically guest_create and guest_find_domid.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

(Not sure why I did it like that.)

Ian.

[PATCH OSSTEST v2 05/18] Toolstack: use get_host_method_object() to manage toolstack selection

[Ian Campbell]

This will allow us to more easily have per-toolstack methods etc.

The previous hash of toolstack parameters is now a blessed object. For
now the callers don't need to change but over the following patches we
will refactor things to use method calls. In particular we will be
aiming to remove Command from the hash and use method calls for
everything.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/TestSupport.pm       | 37 ++++---------------------------------
 Osstest/Toolstack/libvirt.pm | 34 ++++++++++++++++++++++++++++++++++
 Osstest/Toolstack/xend.pm    | 35 +++++++++++++++++++++++++++++++++++
 Osstest/Toolstack/xl.pm      | 35 +++++++++++++++++++++++++++++++++++
 4 files changed, 108 insertions(+), 33 deletions(-)
 create mode 100644 Osstest/Toolstack/libvirt.pm
 create mode 100644 Osstest/Toolstack/xend.pm
 create mode 100644 Osstest/Toolstack/xl.pm

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index b7887bc..df05d8a 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -1862,42 +1862,13 @@ sub guest_vncsnapshot_stash ($$$$) {
     target_getfile_root($ho,100, "$rfile", "$stash/$leaf");
 }
 
-our %toolstacks=
-    ('xend' => {
-        NewDaemons => [qw(xend)],
-        OldDaemonInitd => 'xend',
-        Command => 'xm',
-        CfgPathVar => 'cfgpath',
-        Dom0MemFixed => 1,
-        },
-     'xl' => {
-        NewDaemons => [],
-        Dom0MemFixed => 1,
-        Command => 'xl',
-        CfgPathVar => 'cfgpath',
-	RestoreNeedsConfig => 1,
-        },
-     'libvirt' => {
-        NewDaemons => [qw(libvirtd)],
-        Dom0MemFixed => 1,
-        Command => 'virsh',
-        ExtraPackages => [qw(libnl1 libavahi-client3)],
-        },
-     );
-
 sub toolstack ($) {
     my ($ho) = @_;
     return $ho->{Toolstack} if $ho->{Toolstack};
-    my $tsname= $r{toolstack};
-    $tsname= 'xend' if !defined $tsname;
-    my $ts= $toolstacks{$tsname};
-    die "$tsname ?" unless defined $ts;
-    if (!exists $ts->{Name}) {
-        logm("toolstack $tsname");
-        $ts->{Name}= $tsname;
-    }
-    $ho->{Toolstack} = $ts;
-    return $ts;
+
+    my $tsname= $r{toolstack} || 'xend';
+    $ho->{Toolstack}= get_host_method_object($ho, 'Toolstack', $tsname);
+    return $ho->{Toolstack};
 }
 
 sub authorized_keys () {
diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
new file mode 100644
index 0000000..90fe434
--- /dev/null
+++ b/Osstest/Toolstack/libvirt.pm
@@ -0,0 +1,34 @@
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (C) 2014 Citrix 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/>.
+
+
+package Osstest::Toolstack::libvirt;
+
+use strict;
+use warnings;
+
+sub new {
+    my ($class, $ho, $methname,$asset) = @_;
+    return bless { Name => "libvirt",
+		   Host => $ho,
+		   NewDaemons => [qw(libvirtd)],
+		   Dom0MemFixed => 1,
+		   Command => 'virsh',
+		   ExtraPackages => [qw(libnl1 libavahi-client3)],
+    }, $class;
+}
+
+1;
diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
new file mode 100644
index 0000000..881417d
--- /dev/null
+++ b/Osstest/Toolstack/xend.pm
@@ -0,0 +1,35 @@
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (C) 2014 Citrix 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/>.
+
+
+package Osstest::Toolstack::xend;
+
+use strict;
+use warnings;
+
+sub new {
+    my ($class, $ho, $methname,$asset) = @_;
+    return bless { Name => "xend",
+		   Host => $ho,
+		   NewDaemons => [qw(xend)],
+		   OldDaemonInitd => 'xend',
+		   Command => 'xm',
+		   CfgPathVar => 'cfgpath',
+		   Dom0MemFixed => 1,
+    }, $class;
+}
+
+1;
diff --git a/Osstest/Toolstack/xl.pm b/Osstest/Toolstack/xl.pm
new file mode 100644
index 0000000..0b66201
--- /dev/null
+++ b/Osstest/Toolstack/xl.pm
@@ -0,0 +1,35 @@
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (C) 2014 Citrix 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/>.
+
+
+package Osstest::Toolstack::xl;
+
+use strict;
+use warnings;
+
+sub new {
+    my ($class, $ho, $methname,$asset) = @_;
+    return bless { Name => "xl",
+		   Host => $ho,
+		   NewDaemons => [],
+		   Dom0MemFixed => 1,
+		   Command => 'xl',
+		   CfgPathVar => 'cfgpath',
+		   RestoreNeedsConfig => 1,
+    }, $class;
+}
+
+1;
-- 
2.1.1

Re: [PATCH OSSTEST v2 05/18] Toolstack: use get_host_method_object() to manage toolstack selection

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 05/18] Toolstack: use get_host_method_object() to manage toolstack selection"):
> This will allow us to more easily have per-toolstack methods etc.
> 
> The previous hash of toolstack parameters is now a blessed object. For
> now the callers don't need to change but over the following patches we
> will refactor things to use method calls. In particular we will be
> aiming to remove Command from the hash and use method calls for
> everything.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH OSSTEST v2 06/18] TestSupport: always use xl for generic operations.

[Ian Campbell]

Unless the toolstack is xend (for compatibility with pre-xl Xen versions), when
we use xm.

For several operations in TestSupport.pm the actual toolstack isn't really
relevant, since we want info straight from Xen. For simplicity just use xl (or
xm) in these cases, to avoid needing to implement the following specially for
each toolstack:
  - host_get_free_memory
  - guest_get_state
  - guest_find_domid
  - listing assignable pci devices

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/TestSupport.pm | 13 ++++++++++---
 ts-debian-fixup        |  2 +-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index df05d8a..3f63273 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -936,6 +936,13 @@ sub compress_stashed($) {
 
 #---------- other stuff ----------
 
+sub common_toolstack () {
+    my $tsname= $r{toolstack} || 'xend';
+    my $ts = 'xl';
+    $ts = 'xm' if $tsname eq 'xend';
+    return $ts;
+}
+
 sub host_reboot ($) {
     my ($ho) = @_;
     target_reboot($ho);
@@ -956,7 +963,7 @@ END
 
 sub host_get_free_memory($) {
     my ($ho) = @_;
-    my $toolstack = toolstack($ho)->{Command};
+    my $toolstack = common_toolstack();
     # The line is as followed:
     # free_memory       :   XXXX
     my $info = target_cmd_output_root($ho, "$toolstack info", 10);
@@ -1610,7 +1617,7 @@ sub guest_check_up ($) {
 
 sub guest_get_state ($$) {
     my ($ho,$gho) = @_;
-    my $domains= target_cmd_output_root($ho, toolstack($ho)->{Command}." list");
+    my $domains= target_cmd_output_root($ho, common_toolstack()." list");
     $domains =~ s/^Name.*\n//;
     foreach my $l (split /\n/, $domains) {
         $l =~ m/^(\S+) (?: \s+ \d+ ){3} \s+ ([-a-z]+) \s/x or die "$l ?";
@@ -1815,7 +1822,7 @@ sub guest_find_domid ($$) {
     my ($ho,$gho) = @_;
     return if defined $gho->{Domid};
     my $list= target_cmd_output_root($ho,
-                toolstack($ho)->{Command}." list $gho->{Name}");
+                common_toolstack()." list $gho->{Name}");
     $list =~ m/^(?!Name\s)(\S+)\s+(\d+)\s+(\d+)+(\d+)\s.*$/m
         or die "domain list: $list";
     $1 eq $gho->{Name} or die "domain list name $1 expected $gho->{Name}";
diff --git a/ts-debian-fixup b/ts-debian-fixup
index 11e956c..0dc2d07 100755
--- a/ts-debian-fixup
+++ b/ts-debian-fixup
@@ -142,7 +142,7 @@ sub otherfixupcfg () {
     if (@pcipt) {
         logm("checking passthrough device(s) are assignable: @pcipt");
         my @assignables= split /\n/,
-            target_cmd_output_root($ho, toolstack($ho)->{Command}.
+            target_cmd_output_root($ho, common_toolstack().
                                    " pci-assignable-list");
         foreach my $pcipt (@pcipt) {
             die "not assignable: $pcipt (not in: @assignables)"
-- 
2.1.1

Re: [PATCH OSSTEST v2 06/18] TestSupport: always use xl for generic operations.

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 06/18] TestSupport: always use xl for generic operations."):
> For several operations in TestSupport.pm the actual toolstack isn't really
> relevant, since we want info straight from Xen. For simplicity just use xl (or
> xm) in these cases, to avoid needing to implement the following specially for
> each toolstack:
...
> +sub common_toolstack () {
> +    my $tsname= $r{toolstack} || 'xend';
> +    my $ts = 'xl';
> +    $ts = 'xm' if $tsname eq 'xend';
> +    return $ts;
> +}

Shouldn't this look this up in the host ?  I can imagine scenarios
where we have multiple toolstacks at different times or on different
hosts (eg, to test upgrades or sidegrades).

So I think common_toolstack at the very least should take a $ho.

Ian.

[PATCH OSSTEST v2 07/18] TestSupport: guest_create takes a $ho.

[Ian Campbell]

And looks up the toolstack from it.

This is now consistent with guest_destroy.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/TestSupport.pm         | 5 +++--
 ts-debian-hvm-install          | 7 ++-----
 ts-redhat-install              | 7 ++-----
 ts-rumpuserxen-demo-xenstorels | 2 +-
 4 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 3f63273..616f7ed 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -1330,8 +1330,9 @@ sub guest_destroy ($$) {
 }
 
 sub guest_create ($$) {
-    my ($gho,$toolstack) = @_;
-    target_cmd_root($gho->{Host}, "$toolstack create $gho->{CfgPath}", 100);
+    my ($ho,$gho) = @_;
+    my $ts= toolstack($ho);
+    target_cmd_root($ho, $ts->{Command}." create $gho->{CfgPath}", 100);
 }
 
 
diff --git a/ts-debian-hvm-install b/ts-debian-hvm-install
index 0148eef..f274630 100755
--- a/ts-debian-hvm-install
+++ b/ts-debian-hvm-install
@@ -41,9 +41,6 @@ our $disk_mb= 10000;
 our $guesthost= "$gn.guest.osstest";
 our $gho;
 
-our $toolstack= toolstack($ho)->{Command};
-
-
 sub preseed () {
 
     my $preseed_file = preseed_base('wheezy','',());
@@ -183,7 +180,7 @@ logm("Host has $host_freemem_mb MB free memory, setting guest memory size to $ra
 
 if (!$stage) {
     prep();
-    guest_create($gho,$toolstack);
+    guest_create($ho,$gho);
 } else {
     $gho= selectguest($gn,$gho);
 }
@@ -193,6 +190,6 @@ if ($stage<2) {
 }
 
 guest_editconfig_nocd($gho,$emptyiso);
-guest_create($gho,$toolstack);
+guest_create($ho,$gho);
 guest_await_dhcp_tcp($gho,300);
 guest_check_up($gho);
diff --git a/ts-redhat-install b/ts-redhat-install
index a0b1fab..0b4ee17 100755
--- a/ts-redhat-install
+++ b/ts-redhat-install
@@ -37,9 +37,6 @@ our $disk_mb= 50000;
 our $guesthost= "$gn.guest.osstest";
 our $gho;
 
-our $xl= toolstack($ho)->{Command};
-
-
 sub kickstart () {
     my $cryptpw= '$6$anjRJmBbJcrNJGWN$rqvGUhu8ITjvErdIA5C//w2R6b/6wAjHbaF7XF8u3lZg1XI3StthPIE6UII08scOFwASMOepCGpgtsYeCpjqb.';
     my $authkeys= authorized_keys();
@@ -152,7 +149,7 @@ sub prep () {
 
 if (!$stage) {
     prep();
-    guest_create($gho,$xl);
+    guest_create($ho,$gho);
 } else {
     $gho= selectguest($gn,$gho);
 }
@@ -162,6 +159,6 @@ if ($stage<2) {
 }
 
 guest_editconfig_nocd($gho,$emptyiso);
-guest_create($gho,$xl);
+guest_create($ho,$gho);
 guest_await_dhcp_tcp($gho,300);
 guest_check_up($gho);
diff --git a/ts-rumpuserxen-demo-xenstorels b/ts-rumpuserxen-demo-xenstorels
index 6698848..69a2186 100755
--- a/ts-rumpuserxen-demo-xenstorels
+++ b/ts-rumpuserxen-demo-xenstorels
@@ -40,7 +40,7 @@ sub arrangepreserve () {
 }
 
 sub start () {
-    guest_create($gho,toolstack($ho));
+    guest_create($ho, $gho);
 
     $domid = guest_find_domid($ho, $gho);
 }
-- 
2.1.1

Re: [PATCH OSSTEST v2 07/18] TestSupport: guest_create takes a $ho.

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 07/18] TestSupport: guest_create takes a $ho."):
> And looks up the toolstack from it.
...
>  sub guest_create ($$) {
> -    my ($gho,$toolstack) = @_;
> -    target_cmd_root($gho->{Host}, "$toolstack create $gho->{CfgPath}", 100);
> +    my ($ho,$gho) = @_;

But a $gho has a $ho in $gho->{Host}.  So perhaps you would rather
just eliminate the $toolstack parameter ?

Ian.

[PATCH OSSTEST v2 08/18] Toolstack: Refactor guest lifecycle.

[Ian Campbell]

Implement destory/create as per toolstack methods, including implementing the
libvirt version which previously didn't work. To do this we use the virsh
capability to convert an xl/xm style config file into the correct XML.

xend basically calls into the xl helper since they are compatible.

xl/x, uses ->{Command} which will eventually become private.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/TestSupport.pm       |  5 ++---
 Osstest/Toolstack/libvirt.pm | 20 ++++++++++++++++++++
 Osstest/Toolstack/xend.pm    |  5 +++++
 Osstest/Toolstack/xl.pm      | 13 +++++++++++++
 ts-guest-saverestore         |  2 +-
 ts-guest-start               |  4 +---
 ts-windows-install           |  7 +------
 7 files changed, 43 insertions(+), 13 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 616f7ed..16ab4c6 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -1326,13 +1326,12 @@ sub guest_await_shutdown ($$$) {
 
 sub guest_destroy ($$) {
     my ($ho,$gho) = @_;
-    target_cmd_root($ho, toolstack($ho)->{Command}." destroy $gho->{Name}", 40);
+    toolstack($ho)->destroy($gho);
 }
 
 sub guest_create ($$) {
     my ($ho,$gho) = @_;
-    my $ts= toolstack($ho);
-    target_cmd_root($ho, $ts->{Command}." create $gho->{CfgPath}", 100);
+    toolstack($ho)->create($gho->{CfgPath});
 }
 
 
diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 90fe434..ea83995 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -20,15 +20,35 @@ package Osstest::Toolstack::libvirt;
 use strict;
 use warnings;
 
+use Osstest::TestSupport;
+
 sub new {
     my ($class, $ho, $methname,$asset) = @_;
     return bless { Name => "libvirt",
 		   Host => $ho,
 		   NewDaemons => [qw(libvirtd)],
 		   Dom0MemFixed => 1,
+		   CfgPathVar => 'cfgpath',
 		   Command => 'virsh',
 		   ExtraPackages => [qw(libnl1 libavahi-client3)],
     }, $class;
 }
 
+sub destroy ($$) {
+    my ($self,$gho) = @_;
+    my $gn = $gho->{Name};
+    target_cmd_root($self->{Host}, "virsh destroy $gn", 40);
+}
+
+sub create ($$) {
+    my ($self,$cfg) = @_;
+    my $ho = $self->{Host};
+    my $lcfg = $cfg;
+    $lcfg =~ s,/,-,g;
+    $lcfg = "$ho->{Name}--$lcfg";
+    target_cmd_root($ho, "virsh domxml-from-native xen-xm $cfg > $cfg.xml", 30);
+    target_getfile_root($ho,60,"$cfg.xml", "$stash/$lcfg");
+    target_cmd_root($ho, "virsh create --file $cfg.xml", 100);
+}
+
 1;
diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
index 881417d..c921c20 100644
--- a/Osstest/Toolstack/xend.pm
+++ b/Osstest/Toolstack/xend.pm
@@ -19,6 +19,7 @@ package Osstest::Toolstack::xend;
 
 use strict;
 use warnings;
+use Osstest::Toolstack::xl;
 
 sub new {
     my ($class, $ho, $methname,$asset) = @_;
@@ -32,4 +33,8 @@ sub new {
     }, $class;
 }
 
+# Defer to xl driver for most things
+sub destroy { return &Osstest::Toolstack::xl::destroy; }
+sub create { return &Osstest::Toolstack::xl::create; }
+
 1;
diff --git a/Osstest/Toolstack/xl.pm b/Osstest/Toolstack/xl.pm
index 0b66201..12417ca 100644
--- a/Osstest/Toolstack/xl.pm
+++ b/Osstest/Toolstack/xl.pm
@@ -20,6 +20,8 @@ package Osstest::Toolstack::xl;
 use strict;
 use warnings;
 
+use Osstest::TestSupport;
+
 sub new {
     my ($class, $ho, $methname,$asset) = @_;
     return bless { Name => "xl",
@@ -32,4 +34,15 @@ sub new {
     }, $class;
 }
 
+sub destroy ($$) {
+    my ($self,$gho) = @_;
+    my $gn = $gho->{Name};
+    target_cmd_root($self->{Host}, $self->{Command}." destroy $gn", 40);
+}
+
+sub create ($$) {
+    my ($self,$cfg) = @_;
+    target_cmd_root($self->{Host}, $self->{Command}." create $cfg", 100);
+}
+
 1;
diff --git a/ts-guest-saverestore b/ts-guest-saverestore
index 9e04ae9..8911aed 100755
--- a/ts-guest-saverestore
+++ b/ts-guest-saverestore
@@ -38,7 +38,7 @@ sub restore () {
 		    toolstack($ho)->{Command}
 		    ." restore "
 		    .(toolstack($ho)->{RestoreNeedsConfig} ?
-		      $r{ $gho->{Guest}.'_'. toolstack($ho)->{CfgPathVar} } : '')
+		      $gho->{CfgPath} : '')
 		    ." image", 200);
     target_ping_check_up($gho);
 }
diff --git a/ts-guest-start b/ts-guest-start
index bfbb734..fb6a174 100755
--- a/ts-guest-start
+++ b/ts-guest-start
@@ -26,9 +26,7 @@ our ($ho,$gho) = ts_get_host_guest(@ARGV);
 
 sub start () {
     guest_umount_lv($ho, $gho);
-    my $cmd= toolstack($ho)->{Command}." create ".
-        $r{ $gho->{Guest}.'_'. toolstack($ho)->{CfgPathVar} };
-    target_cmd_root($ho, $cmd, 30);
+    toolstack($ho)->create($r{ $gho->{Guest}.'_'. toolstack($ho)->{CfgPathVar} });
 }
 
 sub checkstart () {
diff --git a/ts-windows-install b/ts-windows-install
index 4b06310..f12a4f4 100755
--- a/ts-windows-install
+++ b/ts-windows-install
@@ -49,13 +49,8 @@ END
     store_runvar("$gho->{Guest}_pingbroken", 1);
 }
 
-sub start () {
-    target_cmd_root($ho, toolstack($ho)->{Command}.
-                    " create $gho->{CfgPath}", 100);
-}
-
 prep();
-start();
+guest_create($ho,$gho);
 
 guest_await_dhcp_tcp($gho,7000);
 guest_check_up($gho);
-- 
2.1.1

Re: [PATCH OSSTEST v2 08/18] Toolstack: Refactor guest lifecycle.

[Ian Campbell]

On Tue, 2014-12-02 at 16:04 +0000, Ian Campbell wrote:
> +sub create ($$) {
> +    my ($self,$cfg) = @_;
> +    my $ho = $self->{Host};
> +    my $lcfg = $cfg;
> +    $lcfg =~ s,/,-,g;
> +    $lcfg = "$ho->{Name}--$lcfg";
> +    target_cmd_root($ho, "virsh domxml-from-native xen-xm $cfg > $cfg.xml", 30);

Given that Jim is in the process of introducing support for "xen-xl" cfg
file consumption I intend to make this xen-xl unconditionally.

Testing older versions of libvirt (which don't speak xen-xl) will
therefore continue to fail at the guest-start test step, as they do
today, and hence it will not appears as a regression.

Testing newer versions of libvirt will pass as expected


Ian.

Re: [PATCH OSSTEST v2 08/18] Toolstack: Refactor guest lifecycle.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH OSSTEST v2 08/18] Toolstack: Refactor guest lifecycle."):
> On Tue, 2014-12-02 at 16:04 +0000, Ian Campbell wrote:
> > +sub create ($$) {
> > +    my ($self,$cfg) = @_;
> > +    my $ho = $self->{Host};
> > +    my $lcfg = $cfg;
> > +    $lcfg =~ s,/,-,g;
> > +    $lcfg = "$ho->{Name}--$lcfg";
> > +    target_cmd_root($ho, "virsh domxml-from-native xen-xm $cfg > $cfg.xml", 30);
> 
> Given that Jim is in the process of introducing support for "xen-xl" cfg
> file consumption I intend to make this xen-xl unconditionally.
> 
> Testing older versions of libvirt (which don't speak xen-xl) will
> therefore continue to fail at the guest-start test step, as they do
> today, and hence it will not appears as a regression.
> 
> Testing newer versions of libvirt will pass as expected

That sounds good, thanks.

Ian.

Re: [PATCH OSSTEST v2 08/18] Toolstack: Refactor guest lifecycle.

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 08/18] Toolstack: Refactor guest lifecycle."):
> Implement destory/create as per toolstack methods, including implementing the
> libvirt version which previously didn't work. To do this we use the virsh
> capability to convert an xl/xm style config file into the correct XML.
> 
> xend basically calls into the xl helper since they are compatible.
> 
> xl/x, uses ->{Command} which will eventually become private.
...
> diff --git a/ts-guest-saverestore b/ts-guest-saverestore
> index 9e04ae9..8911aed 100755
> --- a/ts-guest-saverestore
> +++ b/ts-guest-saverestore
> @@ -38,7 +38,7 @@ sub restore () {
>  		    toolstack($ho)->{Command}
>  		    ." restore "
>  		    .(toolstack($ho)->{RestoreNeedsConfig} ?
> -		      $r{ $gho->{Guest}.'_'. toolstack($ho)->{CfgPathVar} } : '')
> +		      $gho->{CfgPath} : '')
>  		    ." image", 200);

This hunk doesn't seem to be covered by the commit message ?

> -    target_cmd_root($ho, $cmd, 30);
> +    toolstack($ho)->create($r{ $gho->{Guest}.'_'. toolstack($ho)->{CfgPathVar} });

Perhaps the toolstack create method should take the $gho rather than
the config file pathname, like the destroy method does ?

Ian.

Re: [PATCH OSSTEST v2 08/18] Toolstack: Refactor guest lifecycle.

[Ian Campbell]

On Tue, 2015-01-20 at 18:32 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST v2 08/18] Toolstack: Refactor guest lifecycle."):
> > Implement destory/create as per toolstack methods, including implementing the
> > libvirt version which previously didn't work. To do this we use the virsh
> > capability to convert an xl/xm style config file into the correct XML.
> > 
> > xend basically calls into the xl helper since they are compatible.
> > 
> > xl/x, uses ->{Command} which will eventually become private.
> ...
> > diff --git a/ts-guest-saverestore b/ts-guest-saverestore
> > index 9e04ae9..8911aed 100755
> > --- a/ts-guest-saverestore
> > +++ b/ts-guest-saverestore
> > @@ -38,7 +38,7 @@ sub restore () {
> >  		    toolstack($ho)->{Command}
> >  		    ." restore "
> >  		    .(toolstack($ho)->{RestoreNeedsConfig} ?
> > -		      $r{ $gho->{Guest}.'_'. toolstack($ho)->{CfgPathVar} } : '')
> > +		      $gho->{CfgPath} : '')
> >  		    ." image", 200);
> 
> This hunk doesn't seem to be covered by the commit message ?

Indeed, not sure what was going on there. I was a bit confused about the
CfgPathVar during early iterations of the series.

> > -    target_cmd_root($ho, $cmd, 30);
> > +    toolstack($ho)->create($r{ $gho->{Guest}.'_'. toolstack($ho)->{CfgPathVar} });
> 
> Perhaps the toolstack create method should take the $gho rather than
> the config file pathname, like the destroy method does ?

Yes. In fact towards the end of the series I end up arranging that
$gho->{CfgPath} is consistently useful. Since reordering these API
changes is a bit error prone I'd prefer to add a patch onto the end
which reworks the create method's interface to just take $gho. I hope
that's ok.

Ian.

Re: [PATCH OSSTEST v2 08/18] Toolstack: Refactor guest lifecycle.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH OSSTEST v2 08/18] Toolstack: Refactor guest lifecycle."):
> > Perhaps the toolstack create method should take the $gho rather than
> > the config file pathname, like the destroy method does ?
> 
> Yes. In fact towards the end of the series I end up arranging that
> $gho->{CfgPath} is consistently useful. Since reordering these API
> changes is a bit error prone I'd prefer to add a patch onto the end
> which reworks the create method's interface to just take $gho. I hope
> that's ok.

Sure, thanks.

Ian.

[PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Toolstack/libvirt.pm | 6 ++++++
 Osstest/Toolstack/xend.pm    | 1 +
 Osstest/Toolstack/xl.pm      | 6 ++++++
 ts-logs-capture              | 2 +-
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index ea83995..54d2a6d 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -51,4 +51,10 @@ sub create ($$) {
     target_cmd_root($ho, "virsh create --file $cfg.xml", 100);
 }
 
+sub consolecmd ($$) {
+    my ($self,$gho) = @_;
+    my $gn = $gho->{Name};
+    return "virsh console $gn";
+}
+
 1;
diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
index c921c20..896d949 100644
--- a/Osstest/Toolstack/xend.pm
+++ b/Osstest/Toolstack/xend.pm
@@ -36,5 +36,6 @@ sub new {
 # Defer to xl driver for most things
 sub destroy { return &Osstest::Toolstack::xl::destroy; }
 sub create { return &Osstest::Toolstack::xl::create; }
+sub consolecmd { return &Osstest::Toolstack::xl::consolecmd; }
 
 1;
diff --git a/Osstest/Toolstack/xl.pm b/Osstest/Toolstack/xl.pm
index 12417ca..4997775 100644
--- a/Osstest/Toolstack/xl.pm
+++ b/Osstest/Toolstack/xl.pm
@@ -45,4 +45,10 @@ sub create ($$) {
     target_cmd_root($self->{Host}, $self->{Command}." create $cfg", 100);
 }
 
+sub consolecmd ($$) {
+    my ($self,$gho) = @_;
+    my $gn = $gho->{Name};
+    return $self->{Command}." console $gn";
+}
+
 1;
diff --git a/ts-logs-capture b/ts-logs-capture
index 841ad5a..dbca13a 100755
--- a/ts-logs-capture
+++ b/ts-logs-capture
@@ -195,7 +195,7 @@ sub fetch_logs_guest ($) {
         logm("cannot find domid: $@");
         return;
     }
-    my $consolecmd= toolstack($ho)->{Command}." console $gho->{Name}";
+    my $consolecmd= toolstack($ho)->consolecmd($gho);
     try_cmd_output_save("sleep 1 | $consolecmd | cat",
                         "guest-$gho->{Name}-console");
 
-- 
2.1.1

Re: [PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling"):
...
> diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
> index c921c20..896d949 100644
> --- a/Osstest/Toolstack/xend.pm
> +++ b/Osstest/Toolstack/xend.pm
> @@ -36,5 +36,6 @@ sub new {
>  # Defer to xl driver for most things
>  sub destroy { return &Osstest::Toolstack::xl::destroy; }
>  sub create { return &Osstest::Toolstack::xl::create; }
> +sub consolecmd { return &Osstest::Toolstack::xl::consolecmd; }

I hesitate to suggest this, but perhaps Exporter(3pm)'s inheritance
feature (@ISA, I think) might do this more automatically ?

> diff --git a/ts-logs-capture b/ts-logs-capture
> index 841ad5a..dbca13a 100755
> --- a/ts-logs-capture
> +++ b/ts-logs-capture
> @@ -195,7 +195,7 @@ sub fetch_logs_guest ($) {
>          logm("cannot find domid: $@");
>          return;
>      }
> -    my $consolecmd= toolstack($ho)->{Command}." console $gho->{Name}";
> +    my $consolecmd= toolstack($ho)->consolecmd($gho);
>      try_cmd_output_save("sleep 1 | $consolecmd | cat",
>                          "guest-$gho->{Name}-console");

Does `virsh console' work the same way as xl console, particularly wrt
this weird thing with the sleep 1 etc. ?

Ian.

Re: [PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling

[Ian Campbell]

On Tue, 2015-01-20 at 18:34 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling"):
> ...
> > diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
> > index c921c20..896d949 100644
> > --- a/Osstest/Toolstack/xend.pm
> > +++ b/Osstest/Toolstack/xend.pm
> > @@ -36,5 +36,6 @@ sub new {
> >  # Defer to xl driver for most things
> >  sub destroy { return &Osstest::Toolstack::xl::destroy; }
> >  sub create { return &Osstest::Toolstack::xl::create; }
> > +sub consolecmd { return &Osstest::Toolstack::xl::consolecmd; }
> 
> I hesitate to suggest this, but perhaps Exporter(3pm)'s inheritance
> feature (@ISA, I think) might do this more automatically ?

Perhaps, I can't seem to find Exporter(3pm) any where either in Debian
or online. Lots of Exporter::Lite, ::Simple, ::Heavy type stuff
though...

> > diff --git a/ts-logs-capture b/ts-logs-capture
> > index 841ad5a..dbca13a 100755
> > --- a/ts-logs-capture
> > +++ b/ts-logs-capture
> > @@ -195,7 +195,7 @@ sub fetch_logs_guest ($) {
> >          logm("cannot find domid: $@");
> >          return;
> >      }
> > -    my $consolecmd= toolstack($ho)->{Command}." console $gho->{Name}";
> > +    my $consolecmd= toolstack($ho)->consolecmd($gho);
> >      try_cmd_output_save("sleep 1 | $consolecmd | cat",
> >                          "guest-$gho->{Name}-console");
> 
> Does `virsh console' work the same way as xl console, particularly wrt
> this weird thing with the sleep 1 etc. ?

TBH I'm not sure. What is the reason for that weirdness?

Ultimately libvirt/virsh opens the pty directly as opposed to via the
xenconsole binary, AFAICT.

Ian.

Re: [PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling

[Ian Campbell]

On Thu, 2015-01-22 at 11:24 +0000, Ian Campbell wrote:
> On Tue, 2015-01-20 at 18:34 +0000, Ian Jackson wrote:
> > Ian Campbell writes ("[PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling"):
> > ...
> > > diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
> > > index c921c20..896d949 100644
> > > --- a/Osstest/Toolstack/xend.pm
> > > +++ b/Osstest/Toolstack/xend.pm
> > > @@ -36,5 +36,6 @@ sub new {
> > >  # Defer to xl driver for most things
> > >  sub destroy { return &Osstest::Toolstack::xl::destroy; }
> > >  sub create { return &Osstest::Toolstack::xl::create; }
> > > +sub consolecmd { return &Osstest::Toolstack::xl::consolecmd; }
> > 
> > I hesitate to suggest this, but perhaps Exporter(3pm)'s inheritance
> > feature (@ISA, I think) might do this more automatically ?
> 
> Perhaps, I can't seem to find Exporter(3pm) any where either in Debian
> or online. Lots of Exporter::Lite, ::Simple, ::Heavy type stuff
> though...

Turns out that including the 3pm confuses search enginers, searching
just for "perl Exporter" has found me what I need.

Re: [PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling

[Ian Campbell]

On Thu, 2015-01-22 at 11:26 +0000, Ian Campbell wrote:
> On Thu, 2015-01-22 at 11:24 +0000, Ian Campbell wrote:
> > On Tue, 2015-01-20 at 18:34 +0000, Ian Jackson wrote:
> > > Ian Campbell writes ("[PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling"):
> > > ...
> > > > diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
> > > > index c921c20..896d949 100644
> > > > --- a/Osstest/Toolstack/xend.pm
> > > > +++ b/Osstest/Toolstack/xend.pm
> > > > @@ -36,5 +36,6 @@ sub new {
> > > >  # Defer to xl driver for most things
> > > >  sub destroy { return &Osstest::Toolstack::xl::destroy; }
> > > >  sub create { return &Osstest::Toolstack::xl::create; }
> > > > +sub consolecmd { return &Osstest::Toolstack::xl::consolecmd; }
> > > 
> > > I hesitate to suggest this, but perhaps Exporter(3pm)'s inheritance
> > > feature (@ISA, I think) might do this more automatically ?
> > 
> > Perhaps, I can't seem to find Exporter(3pm) any where either in Debian
> > or online. Lots of Exporter::Lite, ::Simple, ::Heavy type stuff
> > though...
> 
> Turns out that including the 3pm confuses search enginers, searching
> just for "perl Exporter" has found me what I need.

It seems that "use parent" is the right way to do this these days, in so
much as the following seems to be recommended and to work. I'll fold it
through the series into the appropriate places.

Ian.

diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
index d76f650..1d5d059 100644
--- a/Osstest/Toolstack/xend.pm
+++ b/Osstest/Toolstack/xend.pm
@@ -19,7 +19,9 @@ package Osstest::Toolstack::xend;
 
 use strict;
 use warnings;
-use Osstest::Toolstack::xl;
+
+# Defer to xl driver for most things
+use parent qw(Osstest::Toolstack::xl);
 
 sub new {
     my ($class, $ho, $methname,$asset) = @_;
@@ -32,15 +34,6 @@ sub new {
     }, $class;
 }
 
-# Defer to xl driver for most things
-sub destroy { return &Osstest::Toolstack::xl::destroy; }
-sub create { return &Osstest::Toolstack::xl::create; }
-sub consolecmd { return &Osstest::Toolstack::xl::consolecmd; }
-sub shutdown_wait { return &Osstest::Toolstack::xl::shutdown_wait; }
-sub migrate { return &Osstest::Toolstack::xl::migrate; }
-sub save { return &Osstest::Toolstack::xl::save; }
-sub restore { return &Osstest::Toolstack::xl::restore; }
-
 # xend always supported migration
 sub migrate_check ($) { return 0; }

Re: [PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling"):
> On Thu, 2015-01-22 at 11:26 +0000, Ian Campbell wrote:
> > Turns out that including the 3pm confuses search enginers, searching
> > just for "perl Exporter" has found me what I need.
> 
> It seems that "use parent" is the right way to do this these days, in so
> much as the following seems to be recommended and to work. I'll fold it
> through the series into the appropriate places.

DYK, how far back is this supported ?

Ian.

Re: [PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling

[Ian Campbell]

On Thu, 2015-01-22 at 14:41 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling"):
> > On Thu, 2015-01-22 at 11:26 +0000, Ian Campbell wrote:
> > > Turns out that including the 3pm confuses search enginers, searching
> > > just for "perl Exporter" has found me what I need.
> > 
> > It seems that "use parent" is the right way to do this these days, in so
> > much as the following seems to be recommended and to work. I'll fold it
> > through the series into the appropriate places.
> 
> DYK, how far back is this supported ?

http://perldoc.perl.org/parent.html says "introduced with Perl
5.004_04.".

Ian.

Re: [PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling"):
> On Thu, 2015-01-22 at 14:41 +0000, Ian Jackson wrote:
> > DYK, how far back is this supported ?
> 
> http://perldoc.perl.org/parent.html says "introduced with Perl
> 5.004_04.".

Wow.  The Perl docs are really out of date - perlmod still has an
example with Exporter.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

(I wouldn't bother folding these changes in unless you feel like it.)

Thanks,
Ian.

Re: [PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling

[Ian Campbell]

On Thu, 2015-01-22 at 15:15 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH OSSTEST v2 09/18] Toolstack: Refactor consolecmd handling"):
> > On Thu, 2015-01-22 at 14:41 +0000, Ian Jackson wrote:
> > > DYK, how far back is this supported ?
> > 
> > http://perldoc.perl.org/parent.html says "introduced with Perl
> > 5.004_04.".
> 
> Wow.  The Perl docs are really out of date - perlmod still has an
> example with Exporter.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks.

> (I wouldn't bother folding these changes in unless you feel like it.)

I already did it...

So I'll take the Ack as permission not to drop the Ack from patches
modified in this way, hope that's ok.

Ian.

[PATCH OSSTEST v2 10/18] Toolstack: Refactor shutdown support

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Toolstack/libvirt.pm | 6 ++++++
 Osstest/Toolstack/xend.pm    | 1 +
 Osstest/Toolstack/xl.pm      | 7 +++++++
 ts-guest-stop                | 5 +----
 4 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 54d2a6d..d039c06 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -57,4 +57,10 @@ sub consolecmd ($$) {
     return "virsh console $gn";
 }
 
+sub shutdown_wait ($$) {
+    my ($self,$gho) = @_;
+    my $gn = $gho->{Name};
+    die "libvirt shutdown wait not implemented yet."
+}
+
 1;
diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
index 896d949..d0e1113 100644
--- a/Osstest/Toolstack/xend.pm
+++ b/Osstest/Toolstack/xend.pm
@@ -37,5 +37,6 @@ sub new {
 sub destroy { return &Osstest::Toolstack::xl::destroy; }
 sub create { return &Osstest::Toolstack::xl::create; }
 sub consolecmd { return &Osstest::Toolstack::xl::consolecmd; }
+sub shutdown_wait { return &Osstest::Toolstack::xl::shutdown_wait; }
 
 1;
diff --git a/Osstest/Toolstack/xl.pm b/Osstest/Toolstack/xl.pm
index 4997775..ce2456b 100644
--- a/Osstest/Toolstack/xl.pm
+++ b/Osstest/Toolstack/xl.pm
@@ -51,4 +51,11 @@ sub consolecmd ($$) {
     return $self->{Command}." console $gn";
 }
 
+sub shutdown_wait ($$) {
+    my ($self,$gho) = @_;
+    my $ho = $self->{Host};
+    my $gn = $gho->{Name};
+    target_cmd_root($ho,"$self->{Command} shutdown -w $gn", 200);
+}
+
 1;
diff --git a/ts-guest-stop b/ts-guest-stop
index 0e3a863..5a10755 100755
--- a/ts-guest-stop
+++ b/ts-guest-stop
@@ -26,10 +26,7 @@ our ($ho,$gho) = ts_get_host_guest(@ARGV);
 
 sub stop () {
     guest_checkrunning($ho, $gho) or die "$gho->{Name} not running";
-    target_cmd_root($ho,
-		    toolstack($ho)->{Command}
-		    ." shutdown -w "
-		    .$gho->{Name}, 200);
+    toolstack($ho)->shutdown_wait($gho);
     guest_checkrunning($ho, $gho) and die $gho->{Name};
 }
 
-- 
2.1.1

Re: [PATCH OSSTEST v2 10/18] Toolstack: Refactor shutdown support

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 10/18] Toolstack: Refactor shutdown support"):
> +sub shutdown_wait ($$) {
> +    my ($self,$gho) = @_;
> +    my $ho = $self->{Host};
> +    my $gn = $gho->{Name};
> +    target_cmd_root($ho,"$self->{Command} shutdown -w $gn", 200);

I think the timeout should be in the callers, not here.  Different
guests might take different times to shut down - but different
toolstacks shouldn't.

Ian.

[PATCH OSSTEST v2 11/18] Toolstack: Refactor migration support check.

[Ian Campbell]

Not implemented for libvirt (the check itself that is, the hook is present).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Toolstack/libvirt.pm |  5 +++++
 Osstest/Toolstack/xend.pm    |  3 +++
 Osstest/Toolstack/xl.pm      |  9 +++++++++
 ts-migrate-support-check     | 10 +---------
 4 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index d039c06..75e49d0 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -63,4 +63,9 @@ sub shutdown_wait ($$) {
     die "libvirt shutdown wait not implemented yet."
 }
 
+sub migrate_check ($) {
+    my ($self) = @_;
+    die "Migration check is not yet supported on libvirt.";
+}
+
 1;
diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
index d0e1113..e69bb1f 100644
--- a/Osstest/Toolstack/xend.pm
+++ b/Osstest/Toolstack/xend.pm
@@ -39,4 +39,7 @@ sub create { return &Osstest::Toolstack::xl::create; }
 sub consolecmd { return &Osstest::Toolstack::xl::consolecmd; }
 sub shutdown_wait { return &Osstest::Toolstack::xl::shutdown_wait; }
 
+# xend always supported migration
+sub migrate_check ($) { return 0; }
+
 1;
diff --git a/Osstest/Toolstack/xl.pm b/Osstest/Toolstack/xl.pm
index ce2456b..3c59da3 100644
--- a/Osstest/Toolstack/xl.pm
+++ b/Osstest/Toolstack/xl.pm
@@ -58,4 +58,13 @@ sub shutdown_wait ($$) {
     target_cmd_root($ho,"$self->{Command} shutdown -w $gn", 200);
 }
 
+sub migrate_check ($) {
+    my ($self) = @_;
+    my $ho = $self->{Host};
+    my $help = target_cmd_output_root($ho, $self->{Command}." help");
+    my $rc = ($help =~ m/^\s*migrate/m) ? 0 : 1;
+    logm("rc=$rc");
+    return $rc;
+}
+
 1;
diff --git a/ts-migrate-support-check b/ts-migrate-support-check
index c70b77a..cd41f68 100755
--- a/ts-migrate-support-check
+++ b/ts-migrate-support-check
@@ -24,12 +24,4 @@ tsreadconfig();
 
 our $ho = selecthost($ARGV[0]);
 
-# all xend/xm platforms support migration
-exit(0) if toolstack($ho)->{Command} eq "xm";
-
-my $help = target_cmd_output_root($ho, toolstack($ho)->{Command}." help");
-
-my $rc = ($help =~ m/^\s*migrate/m) ? 0 : 1;
-
-logm("rc=$rc");
-exit($rc);
+exit(toolstack($ho)->migrate_check());
-- 
2.1.1

Re: [PATCH OSSTEST v2 11/18] Toolstack: Refactor migration support check.

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 11/18] Toolstack: Refactor migration support check."):
> Not implemented for libvirt (the check itself that is, the hook is present).

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support.

[Ian Campbell]

Note that since the previous patch arranges for
ts-migration-support-check to continue to fail for libvirt the libvirt
code is not actually called yet (and will die if it is). This patch is
mainly included to reduce the number of users of
toolstack()->{Command} closer to zero.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Toolstack/libvirt.pm | 5 +++++
 Osstest/Toolstack/xend.pm    | 1 +
 Osstest/Toolstack/xl.pm      | 9 +++++++++
 ts-guest-localmigrate        | 5 +----
 ts-guest-migrate             | 5 +----
 5 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 75e49d0..7740a89 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -68,4 +68,9 @@ sub migrate_check ($) {
     die "Migration check is not yet supported on libvirt.";
 }
 
+sub migrate ($) {
+    my ($self,$gho,$dst,$to) = @_;
+    die "Migration is not yet supported on libvirt.";
+}
+
 1;
diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
index e69bb1f..4b5482f 100644
--- a/Osstest/Toolstack/xend.pm
+++ b/Osstest/Toolstack/xend.pm
@@ -38,6 +38,7 @@ sub destroy { return &Osstest::Toolstack::xl::destroy; }
 sub create { return &Osstest::Toolstack::xl::create; }
 sub consolecmd { return &Osstest::Toolstack::xl::consolecmd; }
 sub shutdown_wait { return &Osstest::Toolstack::xl::shutdown_wait; }
+sub migrate { return &Osstest::Toolstack::xl::migrate; }
 
 # xend always supported migration
 sub migrate_check ($) { return 0; }
diff --git a/Osstest/Toolstack/xl.pm b/Osstest/Toolstack/xl.pm
index 3c59da3..a6f04bc 100644
--- a/Osstest/Toolstack/xl.pm
+++ b/Osstest/Toolstack/xl.pm
@@ -67,4 +67,13 @@ sub migrate_check ($) {
     return $rc;
 }
 
+sub migrate ($$$$) {
+    my ($self,$gho,$dst,$to) = @_;
+    my $ho = $self->{Host};
+    my $gn = $gho->{Name};
+    target_cmd_root($ho,
+		    $self->{Command}." migrate $gn $dst",
+		    $to);
+}
+
 1;
diff --git a/ts-guest-localmigrate b/ts-guest-localmigrate
index f3381da..caefa11 100755
--- a/ts-guest-localmigrate
+++ b/ts-guest-localmigrate
@@ -32,10 +32,7 @@ our ($ho,$gho) = ts_get_host_guest(@ARGV);
 
 sub migrate () {
     guest_checkrunning($ho,$gho) or die $gho->{Name};
-    target_cmd_root($ho,
-		    toolstack($ho)->{Command}
-		    ." migrate $gho->{Name} localhost",
-		    $timeout{Migrate});
+    toolstack($ho)->migrate($gho, "localhost", $timeout{Migrate});
 }
 
 guest_await_dhcp_tcp($gho, 5);
diff --git a/ts-guest-migrate b/ts-guest-migrate
index 65e7b42..55d2854 100755
--- a/ts-guest-migrate
+++ b/ts-guest-migrate
@@ -31,10 +31,7 @@ our $gho = selectguest($ARGV[2],$sho);
 sub migrate () {
     guest_checkrunning($sho,$gho) or die $gho->{Name};
     my $err= guest_check_ip($gho);  die "$err $gho->{Name}" if defined $err;
-    target_cmd_root($sho,
-		    toolstack($sho)->{Command}
-		    ." migrate $gho->{Name} $dho->{Name}",
-		    $timeout{Migrate});
+    toolstack($sho)->migrate($gho, $dho->{Name}, $timeout{Migrate});
 }
 
 guest_await_dhcp_tcp($gho, 5);
-- 
2.1.1

Re: [PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support.

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support."):
> Note that since the previous patch arranges for
> ts-migration-support-check to continue to fail for libvirt the libvirt
> code is not actually called yet (and will die if it is). This patch is
> mainly included to reduce the number of users of
> toolstack()->{Command} closer to zero.
...
> +sub migrate ($$$$) {
> +    my ($self,$gho,$dst,$to) = @_;
> +    my $ho = $self->{Host};
> +    my $gn = $gho->{Name};
> +    target_cmd_root($ho,
> +		    $self->{Command}." migrate $gn $dst",
> +		    $to);
> +}

Again, I think you should pass the $dho rather than the name.  In
general it's better to pass the hash object to these kind of methods.

Ian.

Re: [PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support.

[Ian Campbell]

On Tue, 2015-01-20 at 18:38 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support."):
> > Note that since the previous patch arranges for
> > ts-migration-support-check to continue to fail for libvirt the libvirt
> > code is not actually called yet (and will die if it is). This patch is
> > mainly included to reduce the number of users of
> > toolstack()->{Command} closer to zero.
> ...
> > +sub migrate ($$$$) {
> > +    my ($self,$gho,$dst,$to) = @_;
> > +    my $ho = $self->{Host};
> > +    my $gn = $gho->{Name};
> > +    target_cmd_root($ho,
> > +		    $self->{Command}." migrate $gn $dst",
> > +		    $to);
> > +}
> 
> Again, I think you should pass the $dho rather than the name.  In
> general it's better to pass the hash object to these kind of methods.

For ts-guest-localmigrate is there a cunning stunt you would recommend
to generate a $dho with ->{Name} == "localhost"?

Or perhaps:
        $dho ? $dho->{Name} : "localhost"
? (i.e. $dho==undef => localhost)

Ian.

Re: [PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support."):
> On Tue, 2015-01-20 at 18:38 +0000, Ian Jackson wrote:
> > Again, I think you should pass the $dho rather than the name.  In
> > general it's better to pass the hash object to these kind of methods.
> 
> For ts-guest-localmigrate is there a cunning stunt you would recommend
> to generate a $dho with ->{Name} == "localhost"?
> 
> Or perhaps:
>         $dho ? $dho->{Name} : "localhost"
> ? (i.e. $dho==undef => localhost)

That would do, although it's a big magic.

Magic in a different way would be:

  ->migrate($ts, $gho, { %$dho, Name => 'localhost' }, $timeout)

Ian.

Re: [PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support.

[Ian Campbell]

On Thu, 2015-01-22 at 15:17 +0000, Ian Jackson wrote:
> Ian Campbell writes ("Re: [PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support."):
> > On Tue, 2015-01-20 at 18:38 +0000, Ian Jackson wrote:
> > > Again, I think you should pass the $dho rather than the name.  In
> > > general it's better to pass the hash object to these kind of methods.
> > 
> > For ts-guest-localmigrate is there a cunning stunt you would recommend
> > to generate a $dho with ->{Name} == "localhost"?
> > 
> > Or perhaps:
> >         $dho ? $dho->{Name} : "localhost"
> > ? (i.e. $dho==undef => localhost)
> 
> That would do, although it's a big magic.
> 
> Magic in a different way would be:
> 
>   ->migrate($ts, $gho, { %$dho, Name => 'localhost' }, $timeout)

In the caller? There is no $dho in that case. But I think maybe you
meant $gho? (i.e. this is supposed to produce a copy of $gho with a
different name)

Ian.

Re: [PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support."):
> On Thu, 2015-01-22 at 15:17 +0000, Ian Jackson wrote:
> > Magic in a different way would be:
> > 
> >   ->migrate($ts, $gho, { %$dho, Name => 'localhost' }, $timeout)
> 
> In the caller? There is no $dho in that case. But I think maybe you
> meant $gho? (i.e. this is supposed to produce a copy of $gho with a
> different name)

Yes.  Indeed, I meant $gho.

%$gho expands to a list of (key, value, key, value, ...) which you can
feed to hash constructors like { }.

So { %$gho } is a shallow copy.  And later values for a particular key
in the list override earlier ones.

For some other examples,   git-grep '%\$'

Ian.

Re: [PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support.

[Ian Campbell]

On Thu, 2015-01-22 at 15:19 +0000, Ian Campbell wrote:
> On Thu, 2015-01-22 at 15:17 +0000, Ian Jackson wrote:
> > Ian Campbell writes ("Re: [PATCH OSSTEST v2 12/18] Toolstack: Refactor migration support."):
> > > On Tue, 2015-01-20 at 18:38 +0000, Ian Jackson wrote:
> > > > Again, I think you should pass the $dho rather than the name.  In
> > > > general it's better to pass the hash object to these kind of methods.
> > > 
> > > For ts-guest-localmigrate is there a cunning stunt you would recommend
> > > to generate a $dho with ->{Name} == "localhost"?
> > > 
> > > Or perhaps:
> > >         $dho ? $dho->{Name} : "localhost"
> > > ? (i.e. $dho==undef => localhost)
> > 
> > That would do, although it's a big magic.
> > 
> > Magic in a different way would be:
> > 
> >   ->migrate($ts, $gho, { %$dho, Name => 'localhost' }, $timeout)
> 
> In the caller? There is no $dho in that case. But I think maybe you
> meant $gho? (i.e. this is supposed to produce a copy of $gho with a
> different name)

Actually in this context I think $ho is what was wanted, that's what I
went with.

Ian.

[PATCH OSSTEST v2 13/18] Toolstack: Refactor save/restore support

[Ian Campbell]

Still stubbed out for libvirt.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Toolstack/libvirt.pm |  8 ++++++++
 Osstest/Toolstack/xend.pm    |  2 ++
 Osstest/Toolstack/xl.pm      | 18 ++++++++++++++++++
 ts-guest-saverestore         | 12 ++----------
 4 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 7740a89..8982cc1 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -73,4 +73,12 @@ sub migrate ($) {
     die "Migration is not yet supported on libvirt.";
 }
 
+sub save ($$$$) {
+    die "Save is not yet supported on libvirt.";
+}
+
+sub restore ($$$$$) {
+    die "Restore is not yet supported on libvirt.";
+}
+
 1;
diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
index 4b5482f..6a5f9e6 100644
--- a/Osstest/Toolstack/xend.pm
+++ b/Osstest/Toolstack/xend.pm
@@ -39,6 +39,8 @@ sub create { return &Osstest::Toolstack::xl::create; }
 sub consolecmd { return &Osstest::Toolstack::xl::consolecmd; }
 sub shutdown_wait { return &Osstest::Toolstack::xl::shutdown_wait; }
 sub migrate { return &Osstest::Toolstack::xl::migrate; }
+sub save { return &Osstest::Toolstack::xl::save; }
+sub restore { return &Osstest::Toolstack::xl::restore; }
 
 # xend always supported migration
 sub migrate_check ($) { return 0; }
diff --git a/Osstest/Toolstack/xl.pm b/Osstest/Toolstack/xl.pm
index a6f04bc..adda0c7 100644
--- a/Osstest/Toolstack/xl.pm
+++ b/Osstest/Toolstack/xl.pm
@@ -76,4 +76,22 @@ sub migrate ($$$$) {
 		    $to);
 }
 
+sub save ($$$$) {
+    my ($self,$gho,$f,$to) = @_;
+    my $ho = $self->{Host};
+    my $gn = $gho->{Name};
+    target_cmd_root($ho,$self->{Command}." save $gn $f", $to);
+}
+
+sub restore ($$$$$) {
+    my ($self,$gho,$f,$cfg,$to) = @_;
+    my $ho = $self->{Host};
+    my $gn = $gho->{Name};
+    target_cmd_root($ho,
+		    $self->{Command}
+		    ." restore "
+		    .($self->{RestoreNeedsConfig} ? $cfg : '')
+		    ." $f", $to);
+}
+
 1;
diff --git a/ts-guest-saverestore b/ts-guest-saverestore
index 8911aed..07baa93 100755
--- a/ts-guest-saverestore
+++ b/ts-guest-saverestore
@@ -27,19 +27,11 @@ our ($ho,$gho) = ts_get_host_guest(@ARGV);
 sub save () {
     guest_checkrunning($ho,$gho) or die $gho->{Name};
     my $err= guest_check_ip($gho);  die "$err $gho->{Name}" if defined $err;
-    target_cmd_root($ho,
-		    toolstack($ho)->{Command}
-		    ." save $gho->{Name} image",
-		    200);
+    toolstack($ho)->save($gho,"image",200);
     target_ping_check_down($gho);
 }
 sub restore () {
-    target_cmd_root($ho,
-		    toolstack($ho)->{Command}
-		    ." restore "
-		    .(toolstack($ho)->{RestoreNeedsConfig} ?
-		      $gho->{CfgPath} : '')
-		    ." image", 200);
+    toolstack($ho)->restore($gho,"image",$gho->{CfgPath},200);
     target_ping_check_up($gho);
 }
 
-- 
2.1.1

Re: [PATCH OSSTEST v2 13/18] Toolstack: Refactor save/restore support

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 13/18] Toolstack: Refactor save/restore support"):
> Still stubbed out for libvirt.
...
> +sub restore ($$$$$) {
> +    my ($self,$gho,$f,$cfg,$to) = @_;
> +    my $ho = $self->{Host};
> +    my $gn = $gho->{Name};
> +    target_cmd_root($ho,
> +		    $self->{Command}
> +		    ." restore "
> +		    .($self->{RestoreNeedsConfig} ? $cfg : '')
> +		    ." $f", $to);

Why pass the $cfg explicitly to restore() when we can get it out of
$gho ?  I notice that the timeout here is in the argument list where
it should be.

But, please call the timeout variable `$timeout', not `$to'.  That's
what it's called elsewhere.

Ian.

[PATCH OSSTEST v2 14/18] libvirt: Implement initscript restart which has some hope of working.

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 ts-libvirt-build | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ts-libvirt-build b/ts-libvirt-build
index 940c034..878d4e2 100755
--- a/ts-libvirt-build
+++ b/ts-libvirt-build
@@ -140,8 +140,10 @@ case "$1" in
 	[ "$VERBOSE" != no ] && log_end_msg $?
 	;;
   restart)
-	stop
-	start
+	[ "$VERBOSE" != no ] && log_daemon_msg "Restarting $DESC" "$DAEMON"
+	start-stop-daemon --oknodo --stop --pidfile $PIDFILE --exec $DAEMON
+	start-stop-daemon --start --pidfile $PIDFILE --exec $DAEMON -- -d $libvirtd_opts
+	[ "$VERBOSE" != no ] && log_end_msg $?
 	;;
   reload|force-reload)
 	start-stop-daemon --stop --signal 1 --quiet --pidfile \
-- 
2.1.1

Re: [PATCH OSSTEST v2 14/18] libvirt: Implement initscript restart which has some hope of working.

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 14/18] libvirt: Implement initscript restart which has some hope of working."):
...
>    restart)
> -	stop
> -	start
> +	[ "$VERBOSE" != no ] && log_daemon_msg "Restarting $DESC" "$DAEMON"
> +	start-stop-daemon --oknodo --stop --pidfile $PIDFILE --exec $DAEMON
> +	start-stop-daemon --start --pidfile $PIDFILE --exec $DAEMON -- -d $libvirtd_opts
> +	[ "$VERBOSE" != no ] && log_end_msg $?

Well, the old code is pretty bad because the functions start() and
stop() don't exist.  But wouldn't it be better to move the code into
functions, rather than duplicating it ?

(AFAICT the stop code in restart differs in that it uses --oknodo but
you could pass that to your `stop' function.)

Ian.

[PATCH OSSTEST v2 15/18] libvirt: Implement shutdown_wait

[Ian Campbell]

virsh does not include a --wait option to shutdown as xl and xm do, so
we implement it by hand.

Needs new guest_await_destroy helper. Note the guest_await_shutdown
requires on_shutdown='preserve'

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/TestSupport.pm       | 7 ++++++-
 Osstest/Toolstack/libvirt.pm | 4 +++-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Osstest/TestSupport.pm b/Osstest/TestSupport.pm
index 16ab4c6..456cf49 100644
--- a/Osstest/TestSupport.pm
+++ b/Osstest/TestSupport.pm
@@ -93,7 +93,7 @@ BEGIN {
                       guest_checkrunning guest_check_ip guest_find_ether
                       guest_find_domid guest_check_up guest_check_up_quick
                       guest_get_state guest_await_reboot
-                      guest_await_shutdown guest_destroy
+                      guest_await_shutdown guest_await_destroy guest_destroy
                       guest_vncsnapshot_begin guest_vncsnapshot_stash
 		      guest_check_remus_ok guest_editconfig
                       host_involves_pcipassthrough host_get_pcipassthrough_devs
@@ -1324,6 +1324,11 @@ sub guest_await_shutdown ($$$) {
     return guest_await_state($ho,$gho, "shutdown", "s", $timeout);
 }
 
+sub guest_await_destroy ($$$) {
+    my ($ho,$gho, $timeout) = @_;
+    return guest_await_state($ho,$gho, "destroy", "", $timeout);
+}
+
 sub guest_destroy ($$) {
     my ($ho,$gho) = @_;
     toolstack($ho)->destroy($gho);
diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 8982cc1..c18f467 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -59,8 +59,10 @@ sub consolecmd ($$) {
 
 sub shutdown_wait ($$) {
     my ($self,$gho) = @_;
+    my $ho = $self->{Host};
     my $gn = $gho->{Name};
-    die "libvirt shutdown wait not implemented yet."
+    target_cmd_root($ho, "virsh shutdown $gn", 30);
+    guest_await_destroy($ho,$gho,200);
 }
 
 sub migrate_check ($) {
-- 
2.1.1

Re: [PATCH OSSTEST v2 15/18] libvirt: Implement shutdown_wait

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 15/18] libvirt: Implement shutdown_wait"):
> virsh does not include a --wait option to shutdown as xl and xm do, so
> we implement it by hand.
> 
> Needs new guest_await_destroy helper. Note the guest_await_shutdown
> requires on_shutdown='preserve'

I see that the existing ts-guest-stop doesn't check that the guest
stopped with a correct exit code, which is rather poor.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks,
Ian.

[PATCH OSSTEST v2 16/18] Toolstack: Remove Command field for all toolstacks.

[Ian Campbell]

Nothing in generic code uses this now, so remove.

xl+xend retain as _Command for internal use only.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Toolstack/libvirt.pm |  1 -
 Osstest/Toolstack/xend.pm    |  2 +-
 Osstest/Toolstack/xl.pm      | 18 +++++++++---------
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index c18f467..e50238d 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -29,7 +29,6 @@ sub new {
 		   NewDaemons => [qw(libvirtd)],
 		   Dom0MemFixed => 1,
 		   CfgPathVar => 'cfgpath',
-		   Command => 'virsh',
 		   ExtraPackages => [qw(libnl1 libavahi-client3)],
     }, $class;
 }
diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
index 6a5f9e6..97c6da6 100644
--- a/Osstest/Toolstack/xend.pm
+++ b/Osstest/Toolstack/xend.pm
@@ -27,7 +27,7 @@ sub new {
 		   Host => $ho,
 		   NewDaemons => [qw(xend)],
 		   OldDaemonInitd => 'xend',
-		   Command => 'xm',
+		   _Command => 'xm',
 		   CfgPathVar => 'cfgpath',
 		   Dom0MemFixed => 1,
     }, $class;
diff --git a/Osstest/Toolstack/xl.pm b/Osstest/Toolstack/xl.pm
index adda0c7..5480f8f 100644
--- a/Osstest/Toolstack/xl.pm
+++ b/Osstest/Toolstack/xl.pm
@@ -28,7 +28,7 @@ sub new {
 		   Host => $ho,
 		   NewDaemons => [],
 		   Dom0MemFixed => 1,
-		   Command => 'xl',
+		   _Command => 'xl',
 		   CfgPathVar => 'cfgpath',
 		   RestoreNeedsConfig => 1,
     }, $class;
@@ -37,31 +37,31 @@ sub new {
 sub destroy ($$) {
     my ($self,$gho) = @_;
     my $gn = $gho->{Name};
-    target_cmd_root($self->{Host}, $self->{Command}." destroy $gn", 40);
+    target_cmd_root($self->{Host}, $self->{_Command}." destroy $gn", 40);
 }
 
 sub create ($$) {
     my ($self,$cfg) = @_;
-    target_cmd_root($self->{Host}, $self->{Command}." create $cfg", 100);
+    target_cmd_root($self->{Host}, $self->{_Command}." create $cfg", 100);
 }
 
 sub consolecmd ($$) {
     my ($self,$gho) = @_;
     my $gn = $gho->{Name};
-    return $self->{Command}." console $gn";
+    return $self->{_Command}." console $gn";
 }
 
 sub shutdown_wait ($$) {
     my ($self,$gho) = @_;
     my $ho = $self->{Host};
     my $gn = $gho->{Name};
-    target_cmd_root($ho,"$self->{Command} shutdown -w $gn", 200);
+    target_cmd_root($ho,"$self->{_Command} shutdown -w $gn", 200);
 }
 
 sub migrate_check ($) {
     my ($self) = @_;
     my $ho = $self->{Host};
-    my $help = target_cmd_output_root($ho, $self->{Command}." help");
+    my $help = target_cmd_output_root($ho, $self->{_Command}." help");
     my $rc = ($help =~ m/^\s*migrate/m) ? 0 : 1;
     logm("rc=$rc");
     return $rc;
@@ -72,7 +72,7 @@ sub migrate ($$$$) {
     my $ho = $self->{Host};
     my $gn = $gho->{Name};
     target_cmd_root($ho,
-		    $self->{Command}." migrate $gn $dst",
+		    $self->{_Command}." migrate $gn $dst",
 		    $to);
 }
 
@@ -80,7 +80,7 @@ sub save ($$$$) {
     my ($self,$gho,$f,$to) = @_;
     my $ho = $self->{Host};
     my $gn = $gho->{Name};
-    target_cmd_root($ho,$self->{Command}." save $gn $f", $to);
+    target_cmd_root($ho,$self->{_Command}." save $gn $f", $to);
 }
 
 sub restore ($$$$$) {
@@ -88,7 +88,7 @@ sub restore ($$$$$) {
     my $ho = $self->{Host};
     my $gn = $gho->{Name};
     target_cmd_root($ho,
-		    $self->{Command}
+		    $self->{_Command}
 		    ." restore "
 		    .($self->{RestoreNeedsConfig} ? $cfg : '')
 		    ." $f", $to);
-- 
2.1.1

Re: [PATCH OSSTEST v2 16/18] Toolstack: Remove Command field for all toolstacks.

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 16/18] Toolstack: Remove Command field for all toolstacks."):
> Nothing in generic code uses this now, so remove.
> 
> xl+xend retain as _Command for internal use only.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Although personally I wouldn't bother with the rename.

Ian.

Re: [PATCH OSSTEST v2 16/18] Toolstack: Remove Command field for all toolstacks.

[Ian Campbell]

On Tue, 2015-01-20 at 18:39 +0000, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST v2 16/18] Toolstack: Remove Command field for all toolstacks."):
> > Nothing in generic code uses this now, so remove.
> > 
> > xl+xend retain as _Command for internal use only.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Thanks.

> Although personally I wouldn't bother with the rename.

It was mainly to make it easier to grep for all the prexisting uses of
{Command} which I was trying to eradicate. With that done perhaps it is
redundant, although maybe we want to leave it as _Command so inflight
patch series start using it again...

Ian.

Re: [PATCH OSSTEST v2 16/18] Toolstack: Remove Command field for all toolstacks.

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH OSSTEST v2 16/18] Toolstack: Remove Command field for all toolstacks."):
> On Tue, 2015-01-20 at 18:39 +0000, Ian Jackson wrote:
> > Although personally I wouldn't bother with the rename.
> 
> It was mainly to make it easier to grep for all the prexisting uses of
> {Command} which I was trying to eradicate. With that done perhaps it is
> redundant, although maybe we want to leave it as _Command so inflight
> patch series start using it again...

Mmm, yes.  Ok.

Thanks,
Ian.

[PATCH OSSTEST v2 17/18] ts-guest-start: Use guest_create

[Ian Campbell]

This allows us to abolish CfgPathVar which was inconsistently used,
appears redundant with $gho->{CfgPath} and in any case never set to
anything other than 'cfgpath'.

I suppose it was intended to deal with toolstacks with a cfg format
completely dissimilar to xm/xl's. I think if this arises in a future
toolstack this functionality could be reintroduced pretty trivially
via the toolstack abstraction which is added by this series.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Toolstack/libvirt.pm | 1 -
 Osstest/Toolstack/xend.pm    | 1 -
 Osstest/Toolstack/xl.pm      | 1 -
 ts-guest-start               | 2 +-
 4 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index e50238d..8030aeb 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -28,7 +28,6 @@ sub new {
 		   Host => $ho,
 		   NewDaemons => [qw(libvirtd)],
 		   Dom0MemFixed => 1,
-		   CfgPathVar => 'cfgpath',
 		   ExtraPackages => [qw(libnl1 libavahi-client3)],
     }, $class;
 }
diff --git a/Osstest/Toolstack/xend.pm b/Osstest/Toolstack/xend.pm
index 97c6da6..d76f650 100644
--- a/Osstest/Toolstack/xend.pm
+++ b/Osstest/Toolstack/xend.pm
@@ -28,7 +28,6 @@ sub new {
 		   NewDaemons => [qw(xend)],
 		   OldDaemonInitd => 'xend',
 		   _Command => 'xm',
-		   CfgPathVar => 'cfgpath',
 		   Dom0MemFixed => 1,
     }, $class;
 }
diff --git a/Osstest/Toolstack/xl.pm b/Osstest/Toolstack/xl.pm
index 5480f8f..cd6ba15 100644
--- a/Osstest/Toolstack/xl.pm
+++ b/Osstest/Toolstack/xl.pm
@@ -29,7 +29,6 @@ sub new {
 		   NewDaemons => [],
 		   Dom0MemFixed => 1,
 		   _Command => 'xl',
-		   CfgPathVar => 'cfgpath',
 		   RestoreNeedsConfig => 1,
     }, $class;
 }
diff --git a/ts-guest-start b/ts-guest-start
index fb6a174..19d6830 100755
--- a/ts-guest-start
+++ b/ts-guest-start
@@ -26,7 +26,7 @@ our ($ho,$gho) = ts_get_host_guest(@ARGV);
 
 sub start () {
     guest_umount_lv($ho, $gho);
-    toolstack($ho)->create($r{ $gho->{Guest}.'_'. toolstack($ho)->{CfgPathVar} });
+    guest_create($ho,$gho);
 }
 
 sub checkstart () {
-- 
2.1.1

Re: [PATCH OSSTEST v2 17/18] ts-guest-start: Use guest_create

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 17/18] ts-guest-start: Use guest_create"):
> This allows us to abolish CfgPathVar which was inconsistently used,
> appears redundant with $gho->{CfgPath} and in any case never set to
> anything other than 'cfgpath'.
> 
> I suppose it was intended to deal with toolstacks with a cfg format
> completely dissimilar to xm/xl's. I think if this arises in a future
> toolstack this functionality could be reintroduced pretty trivially
> via the toolstack abstraction which is added by this series.

Very early in libxl's history it couldn't cope with xm-format config
files and had its own format (with mandatory semicolons and various
other differences).  This is now irrelevant.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH OSSTEST v2 18/18] WIP: libvirt: migration + save/restore support.

[Ian Campbell]

Note that this remains stubbed out, since making it actually work
requires more work (i.e. I need to figure out what is involved, seem
to need TLS and a CA etc...)

Appears to need gnutls enabling for migration, even to localhost.

NB haven't managed to get this actually working. With GNUtls enabled
it wants a CA certificate installed:
    error: Cannot read CA certificate '/etc/pki/CA/cacert.pem': No such file or directory

Other attempts give:
    # virsh migrate --live debian.guest.osstest xen://
    error: Requested operation is not valid: domain 'debian.guest.osstest' is already active

Also need to try tcp:// based, by bodging libvirtd.conf to turn on
tcp.

TODO: Ask Jim for advice...

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 Osstest/Toolstack/libvirt.pm | 17 +++++++++++++++++
 ts-libvirt-build             |  3 +++
 ts-xen-build-prep            |  1 +
 3 files changed, 21 insertions(+)

diff --git a/Osstest/Toolstack/libvirt.pm b/Osstest/Toolstack/libvirt.pm
index 8030aeb..e427f63 100644
--- a/Osstest/Toolstack/libvirt.pm
+++ b/Osstest/Toolstack/libvirt.pm
@@ -65,12 +65,29 @@ sub shutdown_wait ($$) {
 
 sub migrate_check ($) {
     my ($self) = @_;
+
+    # Localhost migration currently requires more setup on our side
+    # than we currently do. Once that is sorted out the code below
+    # will correctly determine whether the version of libvirt under
+    # test even supports the possibility.
     die "Migration check is not yet supported on libvirt.";
+
+    my $ho = $self->{Host};
+    my $caps = target_cmd_output_root($ho, "virsh capabilities");
+    my $rc = ($caps =~ m/<migration_features>/) ? 0 : 1;
+    logm("rc=$rc");
+    return $rc;
 }
 
 sub migrate ($) {
     my ($self,$gho,$dst,$to) = @_;
     die "Migration is not yet supported on libvirt.";
+
+    my $ho = $self->{Host};
+    my $gn = $gho->{Name};
+    target_cmd_root($ho,
+		    "virsh migrate $gn $dst",
+		    $to);
 }
 
 sub save ($$$$) {
diff --git a/ts-libvirt-build b/ts-libvirt-build
index 878d4e2..c160533 100755
--- a/ts-libvirt-build
+++ b/ts-libvirt-build
@@ -55,6 +55,7 @@ sub config() {
             ./autogen.sh --no-git \\
                          --with-libxl --without-xen --without-xenapi --without-selinux \\
                          --without-lxc --without-vbox --without-uml \\
+                         --with-gnutls \\
                          --sysconfdir=/etc --localstatedir=/var #/
 END
 }
@@ -80,6 +81,8 @@ END
                                  $builddir.'/dist/etc/init.d/libvirtd');
     target_cmd_build($ho, 60, $builddir, <<END);
         chmod +x $builddir/dist/etc/init.d/libvirtd
+	#sed -i -e 's/\#\?listen_tls = .*/listen_tls = 0/g' $builddir/dist/etc/libvirt/libvirtd.conf
+	#sed -i -e 's/\#\?listen_tcp = .*/listen_tcp = 1/g' $builddir/dist/etc/libvirt/libvirtd.conf
 END
 }
 
diff --git a/ts-xen-build-prep b/ts-xen-build-prep
index 05a7857..ae60b30 100755
--- a/ts-xen-build-prep
+++ b/ts-xen-build-prep
@@ -174,6 +174,7 @@ sub prep () {
                                libpci-dev libncurses5-dev libssl-dev python-dev
                                libx11-dev git-core uuid-dev gettext gawk
                                libsdl-dev libyajl-dev libaio-dev libpixman-1-dev
+                               libgnutls-dev
                                libglib2.0-dev liblzma-dev pkg-config
                                autoconf automake libtool xsltproc
                                libxml2-utils libxml2-dev libnl-dev
-- 
2.1.1

Re: [PATCH OSSTEST v2 18/18] WIP: libvirt: migration + save/restore support.

[Wei Liu]

On Tue, Dec 02, 2014 at 04:04:59PM +0000, Ian Campbell wrote:
> Note that this remains stubbed out, since making it actually work
> requires more work (i.e. I need to figure out what is involved, seem
> to need TLS and a CA etc...)
> 
> Appears to need gnutls enabling for migration, even to localhost.
> 
> NB haven't managed to get this actually working. With GNUtls enabled
> it wants a CA certificate installed:
>     error: Cannot read CA certificate '/etc/pki/CA/cacert.pem': No such file or directory
> 
> Other attempts give:
>     # virsh migrate --live debian.guest.osstest xen://
>     error: Requested operation is not valid: domain 'debian.guest.osstest' is already active
> 
> Also need to try tcp:// based, by bodging libvirtd.conf to turn on
> tcp.
> 

Tried this. It didn't make any difference with regard to the error
message you saw, i.e. it's the same as above.

I've also played with changing domain name and UUID (since they are what
libvirt is checking), but  it didn't work either. Error messages were
different but libvirt still refuses to migrate to localhost.

On the other hand, save and restore worked just fine. I have a patch to
implement save and restore, which is literally calling "virsh save" and
"virsh restore".

One way to move forward is to separate the concept of migration support
and save / restore support, so that we can test save / restore for
libvirt, even if we can test local migration at the moment.

Of course if we can get local migration to work that would be best.

> TODO: Ask Jim for advice...
> 

Jim, any suggestion?

Wei.

Re: [PATCH OSSTEST v2 18/18] WIP: libvirt: migration + save/restore support.

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST v2 18/18] WIP: libvirt: migration + save/restore support."):
> Note that this remains stubbed out, since making it actually work
> requires more work (i.e. I need to figure out what is involved, seem
> to need TLS and a CA etc...)

How annoying.

> NB haven't managed to get this actually working. With GNUtls enabled
> it wants a CA certificate installed:
>     error: Cannot read CA certificate '/etc/pki/CA/cacert.pem': No such file or directory

Let me pick a nit:

> +    my $ho = $self->{Host};
> +    my $caps = target_cmd_output_root($ho, "virsh capabilities");
> +    my $rc = ($caps =~ m/<migration_features>/) ? 0 : 1;

You should probaby escape the < > with \ in case later Perls make them
metacharacters.

Thanks,
Ian.

Re: [PATCH OSSTEST v2 18/18] WIP: libvirt: migration + save/restore support.

[Jim Fehlig]

Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST v2 18/18] WIP: libvirt: migration + save/restore support."):
>   
>> Note that this remains stubbed out, since making it actually work
>> requires more work (i.e. I need to figure out what is involved, seem
>> to need TLS and a CA etc...)
>>     
>
> How annoying.
>   

You can get around this by disabling TLS in /etc/libvirt/libvirtd.conf.
Then simply use ssh to connect to the dst host, e.g. 'virsh migrate
--live dom xen+ssh://dst-host'.

Alternatively, enable listen_tcp then migrate with 'virsh migrate --live
xen+tcp://dst-host'

> @@ -80,6 +81,8 @@ END
>                                   $builddir.'/dist/etc/init.d/libvirtd');
>      target_cmd_build($ho, 60, $builddir, <<END);
>          chmod +x $builddir/dist/etc/init.d/libvirtd
> +       #sed -i -e 's/\#\?listen_tls = .*/listen_tls = 0/g' 
> $builddir/dist/etc/libvirt/libvirtd.conf
> +       #sed -i -e 's/\#\?listen_tcp = .*/listen_tcp = 1/g' 
> $builddir/dist/etc/libvirt/libvirtd.conf
>  END
>  }
>   

Yes, something like that :-).  I'd disable TLS and use the xen+ssh
connection URI.

Regards,
Jim

Re: [PATCH OSSTEST v2 18/18] WIP: libvirt: migration + save/restore support.

[Ian Campbell]

On Tue, 2015-01-20 at 22:42 -0700, Jim Fehlig wrote:
> Ian Jackson wrote:

Thanks both. I've CCd Wei who has picked up the libvirt migration stuff
based on this series.

> > Ian Campbell writes ("[PATCH OSSTEST v2 18/18] WIP: libvirt: migration + save/restore support."):
> >   
> >> Note that this remains stubbed out, since making it actually work
> >> requires more work (i.e. I need to figure out what is involved, seem
> >> to need TLS and a CA etc...)
> >>     
> >
> > How annoying.
> >   
> 
> You can get around this by disabling TLS in /etc/libvirt/libvirtd.conf.
> Then simply use ssh to connect to the dst host, e.g. 'virsh migrate
> --live dom xen+ssh://dst-host'.
> 
> Alternatively, enable listen_tcp then migrate with 'virsh migrate --live
> xen+tcp://dst-host'
> 
> > @@ -80,6 +81,8 @@ END
> >                                   $builddir.'/dist/etc/init.d/libvirtd');
> >      target_cmd_build($ho, 60, $builddir, <<END);
> >          chmod +x $builddir/dist/etc/init.d/libvirtd
> > +       #sed -i -e 's/\#\?listen_tls = .*/listen_tls = 0/g' 
> > $builddir/dist/etc/libvirt/libvirtd.conf
> > +       #sed -i -e 's/\#\?listen_tcp = .*/listen_tcp = 1/g' 
> > $builddir/dist/etc/libvirt/libvirtd.conf
> >  END
> >  }
> >   
> 
> Yes, something like that :-).  I'd disable TLS and use the xen+ssh
> connection URI.
> 
> Regards,
> Jim
> 

Re: [PATCH OSSTEST v2 18/18] WIP: libvirt: migration + save/restore support. [and 1 more messages]

[Ian Jackson]

Jim Fehlig writes ("Re: [PATCH OSSTEST v2 18/18] WIP: libvirt: migration + save/restore support."):
> You can get around this by disabling TLS in /etc/libvirt/libvirtd.conf.
> Then simply use ssh to connect to the dst host, e.g. 'virsh migrate
> --live dom xen+ssh://dst-host'.

Is this the way people would normally use libvirt to do migration ?

(osstest's xl migration test already uses ssh for migration, but
that's because that's the most convenient and usual way to use
`xl migrate'.)

Ian.

Re: [PATCH OSSTEST v2 18/18] WIP: libvirt: migration + save/restore support. [and 1 more messages]

[Jim Fehlig]

Ian Jackson wrote:
> Jim Fehlig writes ("Re: [PATCH OSSTEST v2 18/18] WIP: libvirt: migration + save/restore support."):
>   
>> You can get around this by disabling TLS in /etc/libvirt/libvirtd.conf.
>> Then simply use ssh to connect to the dst host, e.g. 'virsh migrate
>> --live dom xen+ssh://dst-host'.
>>     
>
> Is this the way people would normally use libvirt to do migration ?
>   

Yes, it is a common approach for the same reasons.  It is convenient and
configured "out of the box".

Regards,
Jim

> (osstest's xl migration test already uses ssh for migration, but
> that's because that's the most convenient and usual way to use
> `xl migrate'.)
>
> Ian.
>
>