[PATCH OSSTEST v3 0/5] Misc standalone wrapper improvements

[PATCH OSSTEST v3 0/5] Misc standalone wrapper improvements

[Ian Campbell]

I've addressed the comments from last time which has resulted in 2 new
patches, one to rename cs-adjust-flight's branch option to branch-set (and
which then contains the docs strings fiddling previously in another patch)
and another to switch to using fail.

Otherwise changes are described in the individual patches.

[PATCH OSSTEST 1/5] cs-adjust-flight: `branch' command ought to be `branch-set'

[Ian Campbell]

Also add a doc string and since this op is not a change adjust the doc
comment accordingly.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v3: New patch.
---
 cs-adjust-flight | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/cs-adjust-flight b/cs-adjust-flight
index 834e2c8..a94ed5f 100755
--- a/cs-adjust-flight
+++ b/cs-adjust-flight
@@ -3,9 +3,9 @@
 # destination flight must already exist
 #
 # args:
-#   <dst-flight> [<change> ...]
+#   <dst-flight> [<op> ...]
 #
-# <change>:
+# <op>:
 #   copy <flight>
 #   copy-jobs <flight> <job-spec>
 #   jobs-list <job-spec>
@@ -16,6 +16,7 @@
 #   runvar-perlop <job-spec> <var-spec> <perl-expr>
 #   recipe-set <job-spec> <new-value>
 #   intended-blessing <intended-blessing>
+#   branch-set <new-branch>
 #
 # <foo-spec>:
 #   <foo-name>
@@ -330,7 +331,7 @@ sub change__intended_blessing {
     verbose "$dstflight blessing set to $blessing\n";
 }
 
-sub change__branch {
+sub change__branch_set {
     die unless @changes >= 1;
     my $branch = shift @changes;
 
-- 
2.5.3

Re: [PATCH OSSTEST 1/5] cs-adjust-flight: `branch' command ought to be `branch-set'

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST 1/5] cs-adjust-flight: `branch' command ought to be `branch-set'"):
> Also add a doc string and since this op is not a change adjust the doc
> comment accordingly.

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

[PATCH OSSTEST 2/5] cs-adjust-flight: Add job-status to report job stats

[Ian Campbell]

The return code of sg-run-job does not reflect the state of the job,
which is instead written to the database. For the benefit of running
tests in a loop until failure add a command to retrieve the status to
stdout.

Add a get-job-status command to the standalone helper script.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v3: Some incidental docs stuff moved to a more appropriate (new)
patch. Only print the job if there might be multiple (i.e. the jobs
argument is some sort of regex)

v2: Replaces "standalone: Add get-job-status to pick status out of
standalone.db" with a variant using cs-adjust-flight
---
 cs-adjust-flight | 19 +++++++++++++++++++
 standalone       | 33 +++++++++++++++++++++++++++++++--
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/cs-adjust-flight b/cs-adjust-flight
index a94ed5f..4bfef48 100755
--- a/cs-adjust-flight
+++ b/cs-adjust-flight
@@ -17,6 +17,7 @@
 #   recipe-set <job-spec> <new-value>
 #   intended-blessing <intended-blessing>
 #   branch-set <new-branch>
+#   job-status <job-spec>
 #
 # <foo-spec>:
 #   <foo-name>
@@ -342,6 +343,24 @@ sub change__branch_set {
     verbose "$dstflight branch set to $branch\n";
 }
 
+sub change__job_status {
+    die unless @changes >= 1;
+    my $jobs = shift @changes;
+
+    my $q = $dbh_tests->prepare(<<END);
+        SELECT status
+          FROM jobs
+         WHERE flight = ? AND job = ?
+END
+    for_jobs($dstflight, $jobs, sub {
+	my ($job) = @_;
+	$q->execute($dstflight, $job);
+	my ($s) = $q->fetchrow_array();
+	print "$job " or die $! if defined spec_re($jobs);
+	print "$s\n" or die $!;
+    });
+}
+
 sub changes () {
     debug("CHANGES...\n");
 
diff --git a/standalone b/standalone
index f64462e..ae70c43 100755
--- a/standalone
+++ b/standalone
@@ -35,6 +35,11 @@ Operations:
   hosts next time. Otherwise osstest will complain if you change the
   host(s) which a job is running on on successive runs.
 
+* get-job-status [cf] [JOB]
+
+  Prints the status (pass, fail, running, etc) of the given job to
+  stdout.
+
 Options:
 
 -c FILE, --config=FILE        Use FILE as configuration file
@@ -140,8 +145,9 @@ if [ $reuse -eq 0 ]; then
     read
 fi
 
-if [ ! -f standalone.db ] ; then
-    echo "No standalone.db? Run standalone-reset." >&2
+db="standalone.db"
+if [ ! -f $db ] ; then
+    echo "No $db? Run standalone-reset." >&2
     exit 1
 fi
 
@@ -199,6 +205,15 @@ with_logging() {
     fi
 }
 
+job_status() {
+    flight=$1; shift
+    job=$1; shift
+
+    status=$(OSSTEST_CONFIG=$config \
+		./cs-adjust-flight $flight job-status $job)
+    echo "$status"
+}
+
 # other potential ops:
 # - run standalone reset
 
@@ -304,6 +319,20 @@ case $op in
 	OSSTEST_JOB=$job \
 	    with_logging logs/$flight/$job.$ts.log ./$ts $hosts $@
 	;;
+
+    get-job-status)
+	need_flight;
+
+	if [ $# -ne 1 ] ; then
+	    echo "get-job-status: Need job" >&2
+	    exit 1
+	fi
+
+	job=$1; shift
+
+	job_status $flight $job
+
+	;;
     *)
 	echo "Unknown op $op" ; exit 1 ;;
 esac
-- 
2.5.3

Re: [PATCH OSSTEST 2/5] cs-adjust-flight: Add job-status to report job stats

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST 2/5] cs-adjust-flight: Add job-status to report job stats"):
> The return code of sg-run-job does not reflect the state of the job,
> which is instead written to the database. For the benefit of running
> tests in a loop until failure add a command to retrieve the status to
> stdout.
...
> +job_status() {
> +    flight=$1; shift
> +    job=$1; shift
> +
> +    status=$(OSSTEST_CONFIG=$config \
> +		./cs-adjust-flight $flight job-status $job)
> +    echo "$status"
> +}

This is rather odd.  Why do you capture the value in a variable and
then pass it to echo ?  You could just let the job_status command
print its output directly.

(Also I might quibble about unparsing the arguments to echo.  Observe
the output of (say) `echo -n'.  printf is often better.  This is only
relevant if $status might start with `-' and if you want to keep the
echo.)

Ian.

Re: [PATCH OSSTEST 2/5] cs-adjust-flight: Add job-status to report job stats

[Ian Campbell]

On Tue, 2015-10-06 at 15:44 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST 2/5] cs-adjust-flight: Add job
> -status to report job stats"):
> > The return code of sg-run-job does not reflect the state of the job,
> > which is instead written to the database. For the benefit of running
> > tests in a loop until failure add a command to retrieve the status to
> > stdout.
> ...
> > +job_status() {
> > +    flight=$1; shift
> > +    job=$1; shift
> > +
> > +    status=$(OSSTEST_CONFIG=$config \
> > +		./cs-adjust-flight $flight job-status $job)
> > +    echo "$status"
> > +}
> 
> This is rather odd.  Why do you capture the value in a variable and
> then pass it to echo ?  You could just let the job_status command
> print its output directly.

It was previously echo "${status#* }" (or something like that). I should
have changed it.

Ian.

[PATCH OSSTEST 3/5] standalone: Check job status at end of run-job.

[Ian Campbell]

Check if the job passed and if not (so status is fail, broken, running
etc) then return an error.

This is convenient for scripting.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
 standalone | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/standalone b/standalone
index ae70c43..3408d24 100755
--- a/standalone
+++ b/standalone
@@ -301,6 +301,11 @@ case $op in
 	OSSTEST_HOST_REUSE=$reuse \
 	OSSTEST_SIMULATE=$dryrun \
 	    with_logging logs/$flight/$job.log ./sg-run-job $job
+
+	status=`job_status $flight $job`
+	echo "$flight.$job status = $status" >&2
+	if [ "x$status" != xpass ] ; then exit 1; fi
+
 	;;
     run-test)
 	need_flight; need_host
-- 
2.5.3

[PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test

[Ian Campbell]

Currently the remainder of the comnand line is passed after the host=
ident, which allows for other idents to be given, which isn't all that
useful in practice.

Instead arrange that any additional options up to a "--" marker are
passed before host= and anything after are passed after.

Since the options themselves have a leading -- this can confuse the
scripts own option parsing, meaning you may need more than one "--"
marker, the first to separate the standalone helper args from the ts
args and a second to separate from any ident optiopns.

./standalone run-test -h $HOST -- test-amd64-amd64-xl-xsm ts-host-install --rescue -- guest=debian

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v3: Use an array var for correct whitespace handling.
---
 standalone | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/standalone b/standalone
index 3408d24..71f656e 100755
--- a/standalone
+++ b/standalone
@@ -318,11 +318,18 @@ case $op in
 	job=$1; shift
 	ts=$1; shift
 
+	options=()
+	for i in $@ ; do
+	    if [ x$i = x-- ] ; then shift; break ; fi
+	    options+=("$i")
+	    shift
+	done
+
 	OSSTEST_CONFIG=$config \
 	OSSTEST_FLIGHT=$flight \
 	OSSTEST_HOST_REUSE=$reuse \
 	OSSTEST_JOB=$job \
-	    with_logging logs/$flight/$job.$ts.log ./$ts $hosts $@
+	    with_logging logs/$flight/$job.$ts.log ./$ts ${options[@]} $hosts $@
 	;;
 
     get-job-status)
-- 
2.5.3

Re: [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test"):
> Currently the remainder of the comnand line is passed after the host=
> ident, which allows for other idents to be given, which isn't all that
> useful in practice.
> 
> Instead arrange that any additional options up to a "--" marker are
> passed before host= and anything after are passed after.
...
> -	    with_logging logs/$flight/$job.$ts.log ./$ts $hosts $@
> +	    with_logging logs/$flight/$job.$ts.log ./$ts ${options[@]} $hosts $@

You mean   ... ./$ts "${options[@]}" $hosts ...
by analogy with  "$@"

mariner:~> a=(a "1  2" b)
mariner:~> echo = ${a[@]}
= a 1 2 b
mariner:~> echo = "${a[@]}"
= a 1  2 b
mariner:~>

Ian.

Re: [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test

[Ian Campbell]

On Tue, 2015-10-06 at 15:46 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST 4/5] standalone: Make it possible to
> pass options to run-test"):
> > Currently the remainder of the comnand line is passed after the host=
> > ident, which allows for other idents to be given, which isn't all that
> > useful in practice.
> > 
> > Instead arrange that any additional options up to a "--" marker are
> > passed before host= and anything after are passed after.
> ...
> > -	    with_logging logs/$flight/$job.$ts.log ./$ts $hosts $@
> > +	    with_logging logs/$flight/$job.$ts.log ./$ts ${options[@]}
> > $hosts $@
> 
> You mean   ... ./$ts "${options[@]}" $hosts ...
> by analogy with  "$@"

We (well, I, since I wrote that) don't use "$@" above but just the bare $@,
which I copied. I suppose that one is wrong too?


> mariner:~> a=(a "1  2" b)
> mariner:~> echo = ${a[@]}
> = a 1 2 b
> mariner:~> echo = "${a[@]}"
> = a 1  2 b
> mariner:~>
> 
> Ian.

Re: [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test"):
> On Tue, 2015-10-06 at 15:46 +0100, Ian Jackson wrote:
> > You mean   ... ./$ts "${options[@]}" $hosts ...
> > by analogy with  "$@"
> 
> We (well, I, since I wrote that) don't use "$@" above but just the bare $@,
> which I copied. I suppose that one is wrong too?

Yes, indeed.  Sorry for not spotting that earlier.

Ian.

Re: [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test

[Ian Campbell]

On Tue, 2015-10-06 at 15:59 +0100, Ian Campbell wrote:
> On Tue, 2015-10-06 at 15:46 +0100, Ian Jackson wrote:
> > Ian Campbell writes ("[PATCH OSSTEST 4/5] standalone: Make it possible
> > to
> > pass options to run-test"):
> > > Currently the remainder of the comnand line is passed after the host=
> > > ident, which allows for other idents to be given, which isn't all
> > > that
> > > useful in practice.
> > > 
> > > Instead arrange that any additional options up to a "--" marker are
> > > passed before host= and anything after are passed after.
> > ...
> > > -	    with_logging logs/$flight/$job.$ts.log ./$ts $hosts $@
> > > +	    with_logging logs/$flight/$job.$ts.log ./$ts
> > > ${options[@]}
> > > $hosts $@
> > 
> > You mean   ... ./$ts "${options[@]}" $hosts ...
> > by analogy with  "$@"
> 
> We (well, I, since I wrote that) don't use "$@" above but just the bare
> $@,
> which I copied. I suppose that one is wrong too?

I've decided it is and will insert a patch to fixup various unquoted uses
of $@.

Also in this patch was
+       for i in $@ ; do
which is similarly wrong, I think.

Re: [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH OSSTEST 4/5] standalone: Make it possible to pass options to run-test"):
> Also in this patch was
> +       for i in $@ ; do
> which is similarly wrong, I think.

I should have spotted that too...

Ian.

[PATCH OSSTEST 5/5] standalone: Use fail() from mgi-common in most places

[Ian Campbell]

Functional change is simply to prepend "$0: ", to change the exit
code for unknown operation and to slightly alter the error message
when no arguments are given.

A few "exit 0" and "exit $rc" remain.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
v3: New patch
---
 standalone | 42 ++++++++++++++++--------------------------
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/standalone b/standalone
index 71f656e..dfe631c 100755
--- a/standalone
+++ b/standalone
@@ -58,10 +58,11 @@ Options:
 EOF
 }
 
+. ./mgi-common
+
 if [ $# -lt 1 ] ; then
-    echo "Need an operation" >&2
     usage
-    exit 1
+    fail "No arguments given"
 fi
 
 op=$1 ; shift
@@ -123,7 +124,7 @@ while true ; do
 	--baseline)nobaseline=n; shift 1;;
 	--help)      usage; exit 0;;
         --) shift ; break ;;
-        *) echo "Internal error!" ; exit 1 ;;
+        *) fail "Internal error!" ;;
 
     esac
 done
@@ -147,34 +148,29 @@ fi
 
 db="standalone.db"
 if [ ! -f $db ] ; then
-    echo "No $db? Run standalone-reset." >&2
-    exit 1
+    fail "No $db? Run standalone-reset."
 fi
 
 if [ -z "$config" ] ; then
-    echo "No config specified." >&2
-    exit 1
+    fail "No config specified."
 fi
 IFS_saved=$IFS
 IFS=:
 for c in $config ; do
     if [ -z "$c" -o ! -r "$c" ] ; then
-        echo "Cannot read config $c." >&2
-        exit 1
+        fail "Cannot read config $c."
     fi
 done
 IFS=$IFS_saved
 
 need_flight() {
     if [ -z "$flight" ] ; then
-	echo "run-job: Need a flight" >&2
-	exit 1
+	fail "run-job: Need a flight"
     fi
 }
 need_host() {
     if [ "x$hosts" = x ] ; then
-	echo "run-job: Need a host" >&2
-	exit 1
+	fail "run-job: Need a host"
     fi
 }
 
@@ -228,8 +224,7 @@ case $op in
         need_flight
 
         if [ $# -lt 1 ] ; then
-            echo "make-flight: Need branch" >&2
-            exit 1
+            fail "make-flight: Need branch"
         fi
 
         branch=$1; shift
@@ -248,8 +243,7 @@ case $op in
 	need_flight
 
 	if [ $# -lt 1 ] ; then
-	    echo "set-paths: Need job" >&2
-	    exit 1
+	    fail "set-paths: Need job"
 	fi
 
 	job=$1; shift
@@ -276,8 +270,7 @@ case $op in
 	need_flight;
 
 	if [ $# -lt 1 ] ; then
-	    echo "run-job: Need job" >&2
-	    exit 1
+	    fail "run-job: Need job"
 	fi
 
 	job=$1; shift
@@ -288,8 +281,7 @@ case $op in
 	need_flight; need_host
 
 	if [ $# -lt 1 ] ; then
-	    echo "run-job: Need job" >&2
-	    exit 1
+	    fail "run-job: Need job"
 	fi
 
 	job=$1; shift
@@ -311,8 +303,7 @@ case $op in
 	need_flight; need_host
 
 	if [ $# -lt 2 ] ; then
-	    echo "run-test: Need job + test" >&2
-	    exit 1
+	    fail "run-test: Need job + test"
 	fi
 
 	job=$1; shift
@@ -336,8 +327,7 @@ case $op in
 	need_flight;
 
 	if [ $# -ne 1 ] ; then
-	    echo "get-job-status: Need job" >&2
-	    exit 1
+	    fail "get-job-status: Need job"
 	fi
 
 	job=$1; shift
@@ -346,5 +336,5 @@ case $op in
 
 	;;
     *)
-	echo "Unknown op $op" ; exit 1 ;;
+	fail "Unknown op $op" ;;
 esac
-- 
2.5.3

Re: [PATCH OSSTEST 5/5] standalone: Use fail() from mgi-common in most places

[Ian Jackson]

Ian Campbell writes ("[PATCH OSSTEST 5/5] standalone: Use fail() from mgi-common in most places"):
> Functional change is simply to prepend "$0: ", to change the exit
> code for unknown operation and to slightly alter the error message
> when no arguments are given.
> 
> A few "exit 0" and "exit $rc" remain.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

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