Re: [Xen-devel] [PATCH v2 7/9] ts-livepatch-[install|run]: Install and initial test-cases.

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

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.

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