[v2,7/9] ts-livepatch-[install|run]: Install and initial test-cases.

Commit Message

Konrad Rzeszutek Wilk Dec. 13, 2016, 6:39 a.m. UTC

There are 37 of the test-cases in there. Before we run
any of them we verify that the payload files are present
in /usr/lib/debug.

If so we go through all of the test-cases.

Also as a dependency we only install them if xenltpdist.tar.gz exists.

Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
---
v1: Initial submission.
v2: Add ts-livepatch-install
    Rename ts-livepatch to ts-livepatch-run
    Use target_cmd_root_status instead of target_cmd_root_rc
    Use target_cmd_output_status as well
    Remove tabs, replace with spaces
    Use Perl for string matching :-)
    Use subroutines for more complex string testing
    Use proper style for variable
    Drop parenthesis on cmd.
---
 ts-livepatch-install |  37 ++++++++++++
 ts-livepatch-run     | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 205 insertions(+)
 create mode 100755 ts-livepatch-install
 create mode 100755 ts-livepatch-run

Comments

Ian Jackson Dec. 13, 2016, 5:08 p.m. UTC | #1

Konrad Rzeszutek Wilk writes ("[PATCH v2 7/9] ts-livepatch-[install|run]: Install and initial test-cases."):
> There are 37 of the test-cases in there. Before we run
> any of them we verify that the payload files are present
> in /usr/lib/debug.

> +    # Whether we can actually execute it.
> +    { C => "xen-livepatch list" },
> +    # And we better have a clean slate..
> +    { C => "xen-livepatch list", OutputCheck => sub { return !m/xen_/; } },

This is considerably nicer, I think.  I hope you agree.

What would you think of making your OutputCheck be optionally simply a
regexp ?  So you would be allowed to write:

> +    { C => "xen-livepatch list", OutputCheck => qr{(?!.*xen_)} },

The tradeoff between terseness and simplicity is is a matter of taste.
I always suggest to people ways to be more terse :-).  Up to you.


But:

> +    { C => "xen-livepatch revert xen_hello_world", rc => 256 },

This is not correct.  rc==256 might mean "world has exploded" or
"stoats are nibbling my toes", as well as "this patch is not
installed".

You need to check that the error you got is the one you expect.  There
are two ways to do this: update the xen-livepatch command line utility
to have an exit status which distinguishes different reasons for the
failure; or, do string matching on the error message.

Here you do neither.  I think this complaint applies to all the
entries with rc=>256.

If you choose to do string matching on the error message, you need
something like
  ErrorCheck => qr{patch not installed}
(And you could arrange that ErrorCheck implied rc=>256, if you like
terseness.)

This is all going to make things slightly fiddly, because ErrorCheck
means redirecting 2>&1 (because you can only get stdout from
tcmdout).  But you want to do that only for ErrorCheck because you
don't want OutputCheck to be fed any stderr output.  So ErrorCheck and
OutputCheck become mutually exclusive.

> +        # Default rc is zero.
> +        my $expected_rc = defined($test->{rc}) ? $test->{rc} : 0;

Do this instead:

           my $expected_rc = $test->{rc} // 0;

And frankly, I think then you don't need the comment.  // is the Perl
idiom for supplying a default value.

> +        my $cmd = "set -e;cd /usr/lib/debug;$test->{C}";

Adding a bit of whitespace after each ; will make the debug output
slightly easier to read.

> +        if (defined($test->{OutputCheck})) {

Since OutputCheck will never be defined but falseish, you can skip
`defined' and just write

  +        if ($test->{OutputCheck}) {

> +            $_ = $output;
> +            $rc=$test->{OutputCheck}->();
                  ^^
Please add two spaces there.  Also     ^^  this -> is technically
unnecessary but you may prefer to keep it.

> +            if ($rc ne 1) {
> +                die "FAILED! OutputCheck=$test->{OutputCheck}, input=$output\n";

I would allow OutputCheck to return any truthy value as success.
So do not say `ne 1'.

Also, $test->{OutputCheck} is typically a coderef, and printing
coderefs is not very useful.  (You just get CODE and a hex number
being a Perl internal pointer value.)

More idiomatic would be the (more terse):

> +            $_ = $output;
  +            $test->{OutputCheck}()
                   or die "FAILED $test->{C}, \`$output'";

> +livepatch_check();
> +my $livepatch_result = livepatch_test();
> +logm("Livepatch test returned $livepatch_result");
> +exit $livepatch_result;

Your livepatch_test function now always explicitly returns 0.  The
checking code here is now pointless, and hence so is the explicit
return.

IMO you should remove the `return 0' from livepatch_test, and simply
call it here without checking its return value.  Perl has exceptions
and it is normally good practice to turn errors into exceptions early
and then rely on exception handling to DTRT.  Now that livepatch_test
does that, you can just rely on it.

So simply:

> +livepatch_check();
  +livepatch_test();
  +logm("Livepatch test successful");
  +exit 0;

Ian.

Patch

diff --git a/ts-livepatch-install b/ts-livepatch-install
new file mode 100755
index 0000000..23c4c20
--- /dev/null
+++ b/ts-livepatch-install
@@ -0,0 +1,37 @@ 
+#!/usr/bin/perl -w
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+#
+# 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
+# any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+use strict qw(vars);
+use DBI;
+use Osstest;
+use Osstest::TestSupport;
+
+tsreadconfig();
+
+our ($whhost) = @ARGV;
+$whhost ||= 'host';
+
+our $ho= selecthost($whhost);
+
+sub extract () {
+    my %distpath;
+    target_extract_jobdistpath($ho, "xen", "path_xenlptdist",
+            $r{"$ho->{Ident}_xenbuildjob"} // $r{"xenbuildjob"},
+             \%distpath);
+}
+
+extract();
diff --git a/ts-livepatch-run b/ts-livepatch-run
new file mode 100755
index 0000000..b315d78
--- /dev/null
+++ b/ts-livepatch-run
@@ -0,0 +1,168 @@ 
+#!/usr/bin/perl -w
+# This is part of "osstest", an automated testing framework for Xen.
+# Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+#
+# 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
+# any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+use strict qw(vars);
+use Osstest;
+use POSIX;
+use Osstest::TestSupport;
+
+tsreadconfig();
+
+my @livepatch_files = qw(xen_hello_world.livepatch xen_replace_world.livepatch
+                         xen_bye_world.livepatch xen_nop.livepatch);
+
+my $xen_extra_info;
+my $xen_minor_ver;
+
+sub populate_data {
+    my @lines = split('\n', $_);
+    foreach my $line (@lines) {
+        my ($key, $values) = split /:/, $line;
+        $values = join("", $values);
+        chomp($values);
+        if ($key =~ m/xen_extra/) {
+            $xen_extra_info = $values;
+        }
+        if ($key =~ m/xen_minor/) {
+            $xen_minor_ver = $values;
+        }
+    }
+    return 1 if $xen_extra_info && $xen_minor_ver;
+    return 0;
+}
+
+sub check_for_hello_world {
+    return m/xen_extra/ && m/Hello World/;
+}
+
+sub check_for_stock {
+    return m/xen_extra/ && m/$xen_extra_info/;
+}
+
+# The NOP test-cases makes xen_minor_ver do nothing. Which means its
+# output value should be nothing like before NOPing it.
+sub check_versions {
+    my @lines = split('\n', $_);
+    foreach my $line (@lines) {
+        my ($key, $values) = split /:/, $line;
+        $values = join("", $values);
+        chomp($values);
+        if ($key =~ m/xen_minor/) {
+            if ($values ne $xen_minor_ver ) {
+                return 1;
+            }
+        }
+    }
+    return 0;
+}
+
+my @livepatch_tests = (
+    # Whether we can actually execute it.
+    { C => "xen-livepatch list" },
+    # And we better have a clean slate..
+    { C => "xen-livepatch list", OutputCheck => sub { return !m/xen_/; } },
+    # Sets the default values
+    { C => "xl info", OutputCheck => \&populate_data },
+    # Sanity check that populate_data did its job.
+    { C => "xl info",
+      OutputCheck => \&check_for_stock },
+    # Let it rip!
+    { C => "xen-livepatch revert xen_hello_world", rc => 256 },
+    { C => "xen-livepatch load xen_hello_world.livepatch" },
+    { C => "xen-livepatch load xen_hello_world.livepatch", rc => 256 },
+    { C => "xen-livepatch list",
+      OutputCheck => sub { m/xen_hello_world/ } },
+    { C => "xl info",
+      OutputCheck => \&check_for_hello_world },
+    { C => "xen-livepatch revert xen_hello_world" },
+    { C => "xl info",
+      OutputCheck => \&check_for_stock },
+    { C => "xen-livepatch unload xen_hello_world" },
+    { C => "xen-livepatch unload xen_hello_world", rc => 256 },
+    { C => "xl info",
+      OutputCheck => \&check_for_stock },
+    { C => "xen-livepatch load xen_hello_world.livepatch" },
+    { C => "xl info",
+      OutputCheck => \&check_for_hello_world },
+    { C => "xen-livepatch load xen_bye_world.livepatch" },
+    { C => "xl info",
+      OutputCheck => sub { m/xen_extra/ && m/Bye World/ } },
+    { C => "xen-livepatch upload xen_replace xen_replace_world.livepatch" },
+    { C => "xen-livepatch replace xen_replace" },
+    { C => "xl info",
+      OutputCheck => sub { m/xen_extra/ && m/Hello Again Wo/ } },
+    { C => "xen-livepatch apply xen_hello_world", rc => 256 },
+    { C => "xen-livepatch apply xen_bye_world", rc => 256 },
+    { C => "xen-livepatch apply xen_replace" },
+    { C => "xen-livepatch revert xen_replace" },
+    { C => "xen-livepatch unload xen_replace" },
+    { C => "xen-livepatch unload xen_hello_world" },
+    { C => "xen-livepatch unload xen_bye_world" },
+    { C => "xen-livepatch list",
+      OutputCheck => sub { !m/xen_/ } },
+    { C => "xl info",
+      OutputCheck => \&check_for_stock },
+    { C => "xen-livepatch load xen_nop.livepatch" },
+    { C => "xen-livepatch revert xen_nop" },
+    { C => "xen-livepatch apply xen_nop" },
+    { C => "xl info",
+      OutputCheck => \&check_versions },
+    { C => "xen-livepatch unload xen_nop", rc => 256 },
+    { C => "xen-livepatch revert xen_nop" },
+    { C => "xen-livepatch unload xen_nop" },
+    );
+
+our $ho = selecthost('host');
+
+sub livepatch_test () {
+    logm "Have ".(scalar @livepatch_tests)." test-cases\n";
+    my $rc;
+    my $output;
+
+    foreach my $test (@livepatch_tests) {
+        # Default rc is zero.
+        my $expected_rc = defined($test->{rc}) ? $test->{rc} : 0;
+        my $cmd = "set -e;cd /usr/lib/debug;$test->{C}";
+        my ($rc, $output) = target_cmd_output_root_status($ho, $cmd);
+
+        if ( $rc != $expected_rc ) {
+            logm "FAILED (got $rc, expected: $expected_rc): \n";
+            die $rc;
+        }
+        if (defined($test->{OutputCheck})) {
+            $_ = $output;
+            $rc=$test->{OutputCheck}->();
+            if ($rc ne 1) {
+                die "FAILED! OutputCheck=$test->{OutputCheck}, input=$output\n";
+            }
+        }
+   }
+   return 0;
+}
+
+sub livepatch_check () {
+    foreach my $file (@livepatch_files) {
+        if (!target_file_exists($ho, "/usr/lib/debug/$$file")) {
+            die "$file is missing!\n";
+        }
+    }
+}
+
+livepatch_check();
+my $livepatch_result = livepatch_test();
+logm("Livepatch test returned $livepatch_result");
+exit $livepatch_result;