[kvm-unit-tests PATCH v3 0/6] reduce exit status ambiguity and more

[kvm-unit-tests PATCH v3 0/6] reduce exit status ambiguity and more

[Andrew Jones]

The first three patches makes changes to reduce the exit status ambiguity,
for which kvm-unit-tests is famous. The next couple are a cleanup and a
minor feature enhancement. The last is a feature enhancement bringing
timeouts to unittest runs.

v3: rebased on latest master; now includes powerpc/run changes (specific
    differences with v2 are pointed out in the patches that changed -
    only two of them)
v2: Thanks to Radim for suggestions and help getting the suggestions
    implemented.


Andrew Jones (6):
  x86: clean up exit use, use abort
  run scripts need consistent exit status
  arch-run: reduce return code ambiguity
  cleanup unittests.cfg headers
  runtime: enable some unittest config overriding
  run scripts: add timeout support

 arm/run                   |  7 +++--
 arm/unittests.cfg         | 27 ++++++++++++-------
 lib/x86/desc.c            |  4 +--
 powerpc/run               | 24 +++++++++++++----
 run_tests.sh              |  4 +--
 scripts/arch-run.bash     | 69 +++++++++++++++++++++++++++++++++++++++++++++++
 scripts/functions.bash    |  8 ++++--
 scripts/mkstandalone.sh   | 16 +++++------
 scripts/runtime.bash      | 20 +++++++-------
 x86/README                | 10 ++++---
 x86/hyperv_stimer.c       |  9 +++----
 x86/pku.c                 |  4 +--
 x86/run                   | 14 +++++-----
 x86/smap.c                |  4 +--
 x86/tscdeadline_latency.c |  4 +--
 x86/unittests.cfg         | 22 ++++++++++-----
 x86/vmx.c                 |  2 +-
 17 files changed, 174 insertions(+), 74 deletions(-)
 create mode 100644 scripts/arch-run.bash

-- 
2.4.3

[kvm-unit-tests PATCH v3 1/6] x86: clean up exit use, use abort

[Andrew Jones]

Change all exit()'s for the purpose of aborts to abort()'s. This changes
a handful of exit(-1)'s, which resulted in a 255 exit status (putting
the status in signal territory) to a 127 exit status (just below signal
territory), as 127 is defined as the unittest abort. Yes, stdlib would
use SIGABRT resulting in 134, but, well, we do what we can...

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
---
 lib/x86/desc.c            | 4 ++--
 x86/hyperv_stimer.c       | 9 +++------
 x86/pku.c                 | 4 ++--
 x86/smap.c                | 4 ++--
 x86/tscdeadline_latency.c | 4 ++--
 x86/vmx.c                 | 2 +-
 6 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/lib/x86/desc.c b/lib/x86/desc.c
index 1a80c01283853..dd07d2c91d769 100644
--- a/lib/x86/desc.c
+++ b/lib/x86/desc.c
@@ -54,7 +54,7 @@ static void check_exception_table(struct ex_regs *regs)
         }
     }
     printf("unhandled exception %d\n", regs->vector);
-    exit(7);
+    abort();
 }
 
 static void (*exception_handlers[32])(struct ex_regs *regs);
@@ -78,7 +78,7 @@ void do_handle_exception(struct ex_regs *regs)
 	printf("unhandled cpu exception %d\n", regs->vector);
 	if (regs->vector == 14)
 		printf("PF at %p addr %p\n", regs->rip, read_cr2());
-	exit(7);
+	abort();
 }
 
 #define EX(NAME, N) extern char NAME##_fault;	\
diff --git a/x86/hyperv_stimer.c b/x86/hyperv_stimer.c
index 767459e55cc0b..bf2e4294ddade 100644
--- a/x86/hyperv_stimer.c
+++ b/x86/hyperv_stimer.c
@@ -99,8 +99,7 @@ static void process_stimer_msg(struct svcpu *svcpu,
         msg->header.message_type != HVMSG_NONE) {
         report("invalid Hyper-V SynIC msg type", false);
         report_summary();
-        exit(-1);
-        return;
+        abort();
     }
 
     if (msg->header.message_type == HVMSG_NONE) {
@@ -110,8 +109,7 @@ static void process_stimer_msg(struct svcpu *svcpu,
     if (msg->header.payload_size < sizeof(*payload)) {
         report("invalid Hyper-V SynIC msg payload size", false);
         report_summary();
-        exit(-1);
-        return;
+        abort();
     }
 
     /* Now process timer expiration message */
@@ -119,8 +117,7 @@ static void process_stimer_msg(struct svcpu *svcpu,
     if (payload->timer_index >= ARRAY_SIZE(svcpu->timer)) {
         report("invalid Hyper-V SynIC timer index", false);
         report_summary();
-        exit(-1);
-        return;
+        abort();
     }
     timer = &svcpu->timer[payload->timer_index];
     process_stimer_expired(svcpu, timer, payload->expiration_time,
diff --git a/x86/pku.c b/x86/pku.c
index 0e00b9984d70d..bcc0d5970f8d3 100644
--- a/x86/pku.c
+++ b/x86/pku.c
@@ -90,8 +90,8 @@ int main(int ac, char **av)
     unsigned int pkru_wd = 0x20;
 
     if (!(cpuid_indexed(7, 0).c & (1 << X86_FEATURE_PKU))) {
-        printf("PKU not enabled, exiting\n");
-        exit(1);
+        printf("PKU not enabled, aborting\n");
+        abort();
     }
 
     setup_vm();
diff --git a/x86/smap.c b/x86/smap.c
index d8a7ae82dc00b..69e71864dc9bf 100644
--- a/x86/smap.c
+++ b/x86/smap.c
@@ -92,8 +92,8 @@ int main(int ac, char **av)
 	unsigned long i;
 
 	if (!(cpuid_indexed(7, 0).b & (1 << X86_FEATURE_SMAP))) {
-		printf("SMAP not enabled, exiting\n");
-		exit(1);
+		printf("SMAP not enabled, aborting\n");
+		abort();
 	}
 
 	setup_vm();
diff --git a/x86/tscdeadline_latency.c b/x86/tscdeadline_latency.c
index 42f19cb92e518..c5f0b06f863d3 100644
--- a/x86/tscdeadline_latency.c
+++ b/x86/tscdeadline_latency.c
@@ -98,8 +98,8 @@ static void test_tsc_deadline_timer(void)
     if(enable_tsc_deadline_timer()) {
         printf("tsc deadline timer enabled\n");
     } else {
-        printf("tsc deadline timer not detected\n");
-        exit(1);
+        printf("tsc deadline timer not detected, aborting\n");
+        abort();
     }
 }
 
diff --git a/x86/vmx.c b/x86/vmx.c
index 3fa1a735881f7..a9cebcb6bdc0c 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -869,7 +869,7 @@ static int exit_handler()
 			, ret);
 	}
 	print_vmexit_info();
-	exit(-1);
+	abort();
 	return 0;
 }
 
-- 
2.4.3

[kvm-unit-tests PATCH v3 2/6] run scripts need consistent exit status

[Andrew Jones]

When a script fails on something build related, e.g. ./configure
hasn't been run, or usage related, e.g. a bad parameter, then an
exit status of 1 makes sense, as no test was attempted. However,
if a test is attempted, then we now need to reserve an exit status
of 1 for QEMU failures. arch-run scripts and standalone generated
run scripts should always use 2, as it's assumed that attempting
to run those attempt to run the test.

(Additional cleanup; remove return values from a couple of
 "void" functions in scripts/mkstandalone)

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
---
 run_tests.sh            |  4 ++--
 scripts/mkstandalone.sh | 14 ++++++--------
 scripts/runtime.bash    |  4 ++--
 x86/run                 |  6 ++----
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/run_tests.sh b/run_tests.sh
index 2c8af36b2726e..10de5474a0b42 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -4,7 +4,7 @@ verbose="no"
 
 if [ ! -f config.mak ]; then
     echo "run ./configure && make first. See ./configure -h"
-    exit
+    exit 1
 fi
 source config.mak
 source scripts/functions.bash
@@ -41,7 +41,7 @@ while getopts "g:hv" opt; do
             verbose="yes"
             ;;
         *)
-            exit
+            exit 1
             ;;
     esac
 done
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index c7e78f83fb086..408bb05480b11 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -2,7 +2,7 @@
 
 if [ ! -f config.mak ]; then
 	echo "run ./configure && make first. See ./configure -h"
-	exit
+	exit 1
 fi
 source config.mak
 source scripts/functions.bash
@@ -21,7 +21,7 @@ temp_file ()
 
 	echo "$var=\`mktemp\`"
 	echo "cleanup=\"\$$var \$cleanup\""
-	echo "base64 -d << 'BIN_EOF' | zcat > \$$var || exit 1"
+	echo "base64 -d << 'BIN_EOF' | zcat > \$$var || exit 2"
 
 	gzip - < $file | base64
 
@@ -47,8 +47,8 @@ generate_test ()
 
 	if [ ! -f $kernel ]; then
 		echo 'echo "skip '"$testname"' (test kernel not present)"'
-		echo 'exit 1'
-		return 1
+		echo 'exit 2'
+		return
 	fi
 
 	echo "trap 'rm -f \$cleanup' EXIT"
@@ -73,11 +73,11 @@ function mkstandalone()
 	local testname="$1"
 
 	if [ -z "$testname" ]; then
-		return 1
+		return
 	fi
 
 	if [ -n "$one_testname" ] && [ "$testname" != "$one_testname" ]; then
-		return 1
+		return
 	fi
 
 	standalone=tests/$testname
@@ -86,8 +86,6 @@ function mkstandalone()
 
 	chmod +x $standalone
 	echo Written $standalone.
-
-	return 0
 }
 
 trap 'rm -f $cfg' EXIT
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 63d1b9653007b..04adb9d070f13 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -23,7 +23,7 @@ function run()
 
     if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
         echo "skip $1 ($arch only)"
-        return
+        return 2
     fi
 
     # check a file for a particular value before running a test
@@ -34,7 +34,7 @@ function run()
         value=${check_param#*=}
         if [ "$path" ] && [ "$(cat $path)" != "$value" ]; then
             echo "skip $1 ($path not equal to $value)"
-            return
+            return 2
         fi
     done
 
diff --git a/x86/run b/x86/run
index bdd7c7e394e45..22d7f2cad0d06 100755
--- a/x86/run
+++ b/x86/run
@@ -1,6 +1,4 @@
 #!/bin/bash
-NOTFOUND=1
-TESTDEVNOTSUPP=2
 
 qemubinarysearch="${QEMU:-qemu-kvm qemu-system-x86_64}"
 
@@ -19,11 +17,11 @@ done
 if      [ -z "${QEMUFOUND}" ]
 then
 	echo "A QEMU binary was not found, You can set a custom location by using the QEMU=<path> environment variable "
-	exit ${NOTFOUND}
+	exit 2
 elif    [ -z "${qemu}" ]
 then
 	echo "No Qemu test device support found"
-	exit ${TESTDEVNOTSUPP}
+	exit 2
 fi
 
 if
-- 
2.4.3

[kvm-unit-tests PATCH v3 3/6] arch-run: reduce return code ambiguity

[Andrew Jones]

qemu/unittest exit codes are convoluted, causing codes 0 and 1
to be ambiguous. Here are the possible meanings

 .-----------------------------------------------------------------.
 |                | 0                                 | 1          |
 |-----------------------------------------------------------------|
 | QEMU           | did something successfully,       | FAILURE    |
 |                | but probably didn't run the       |            |
 |                | unittest, OR caught SIGINT,       |            |
 |                | SIGHUP, or SIGTERM                |            |
 |-----------------------------------------------------------------|
 | unittest       | for some reason exited using      | SUCCESS    |
 |                | ACPI/PSCI, not with debug-exit    |            |
 .-----------------------------------------------------------------.

As we can see above, an exit code of zero is even ambiguous for each
row, i.e. QEMU could exit with zero because it successfully completed,
or because it caught a signal. unittest could exit with zero because
it successfully powered-off, or because for some odd reason it powered-
off instead of calling debug-exit.

And, the most fun is that exit-code == 1 means QEMU failed, but the
unittest succeeded.

This patch attempts to reduce that ambiguity, by also looking at stderr.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

---
v3: now also powerpc/run (which is a bit different due to already having
    one exit code fixup in place)

 arm/run                 |  6 ++---
 powerpc/run             | 23 +++++++++++++++----
 scripts/arch-run.bash   | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
 scripts/mkstandalone.sh |  2 +-
 scripts/runtime.bash    |  2 +-
 x86/README              | 10 ++++----
 x86/run                 |  7 +++---
 7 files changed, 92 insertions(+), 19 deletions(-)
 create mode 100644 scripts/arch-run.bash

diff --git a/arm/run b/arm/run
index dc0a33204c20b..c9463de6dd241 100755
--- a/arm/run
+++ b/arm/run
@@ -6,6 +6,7 @@ if [ -z "$STANDALONE" ]; then
 		exit 2
 	fi
 	source config.mak
+	source scripts/arch-run.bash
 fi
 processor="$PROCESSOR"
 
@@ -71,7 +72,4 @@ command="$qemu $M -cpu $processor $chr_testdev"
 command+=" -display none -serial stdio -kernel"
 echo $command "$@"
 
-$command "$@"
-ret=$?
-echo Return value from qemu: $ret
-exit $ret
+exit_fixup $command "$@"
diff --git a/powerpc/run b/powerpc/run
index 45492a1cb8afc..8ac684cc7c12f 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -6,6 +6,7 @@ if [ -z "$STANDALONE" ]; then
 		exit 2
 	fi
 	source config.mak
+	source scripts/arch-run.bash
 fi
 
 if [ -c /dev/kvm ]; then
@@ -46,10 +47,22 @@ command="$qemu $M -bios $FIRMWARE"
 command+=" -display none -serial stdio -kernel"
 echo $command "$@"
 
-#FIXME: rtas-poweroff always exits with zero, so we have to parse
-#       the true exit code from the output.
-lines=$($command "$@")
+# powerpc tests currently exit with rtas-poweroff, which exits with 0.
+# exit_fixup treats that as a failure exit and returns 1, so we need
+# to fixup the fixup below by parsing the true exit code from the output.
+# The second fixup is also a FIXME, because once we add chr-testdev
+# support for powerpc, we won't need the second fixup.
+lines=$(exit_fixup $command "$@")
+ret=$?
 echo "$lines"
-ret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/')
-echo Return value from qemu: $ret
+if [ $ret -eq 1 ]; then
+	testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/')
+	if [ "$testret" ]; then
+		if [ $testret -eq 1 ]; then
+			ret=0
+		else
+			ret=$testret
+		fi
+	fi
+fi
 exit $ret
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
new file mode 100644
index 0000000000000..3a63ed179a7de
--- /dev/null
+++ b/scripts/arch-run.bash
@@ -0,0 +1,61 @@
+##############################################################################
+# exit_fixup translates the ambiguous exit status in Table1 to that in Table2.
+# Table3 simply documents the complete status table.
+#
+# Table1: Before fixup
+# --------------------
+# 0      - Unexpected exit from QEMU (possible signal), or the unittest did
+#          not use debug-exit
+# 1      - most likely unittest succeeded, or QEMU failed
+#
+# Table2: After fixup
+# -------------------
+# 0      - Everything succeeded
+# 1      - most likely QEMU failed
+#
+# Table3: Complete table
+# ----------------------
+# 0      - SUCCESS
+# 1      - most likely QEMU failed
+# 2      - most likely a run script failed
+# 3      - most likely the unittest failed
+# 127    - most likely the unittest called abort()
+# 1..127 - FAILURE (could be QEMU, a run script, or the unittest)
+# >= 128 - Signal (signum = status - 128)
+##############################################################################
+exit_fixup ()
+{
+	local stdout errors ret sig
+
+	exec {stdout}>&1
+	errors=$("${@}" 2>&1 1>&${stdout} | tee >(cat - 1>&2); exit ${PIPESTATUS[0]})
+	ret=$?
+	exec {stdout}>&-
+
+	if [ "$errors" ]; then
+		sig=$(grep 'terminating on signal' <<<"$errors")
+		if [ "$sig" ]; then
+			sig=$(sed 's/.*terminating on signal \([0-9][0-9]*\).*/\1/' <<<"$sig")
+		fi
+	fi
+
+	if [ $ret -eq 0 ]; then
+		# Some signals result in a zero return status, but the
+		# error log tells the truth.
+		if [ "$sig" ]; then
+			((ret=sig+128))
+		else
+			# Exiting with zero (non-debugexit) is an error
+			ret=1
+		fi
+	elif [ $ret -eq 1 ]; then
+		# Even when ret==1 (unittest success) if we also got stderr
+		# logs, then we assume a QEMU failure. Otherwise we translate
+		# status of 1 to 0 (SUCCESS)
+		if [ -z "$errors" ]; then
+			ret=0
+		fi
+	fi
+
+	return $ret
+}
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index 408bb05480b11..4c201aa9a4743 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -61,7 +61,7 @@ generate_test ()
 	temp_file bin "$kernel"
 	args[3]='$bin'
 
-	temp_file RUNTIME_arch_run "$TEST_DIR/run"
+	temp_file RUNTIME_arch_run <(echo "#!/bin/bash"; cat scripts/arch-run.bash "$TEST_DIR/run")
 
 	cat scripts/runtime.bash
 
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 04adb9d070f13..9c973f26f8de6 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -48,7 +48,7 @@ function run()
     eval $cmdline
     ret=$?
 
-    if [ $ret -le 1 ]; then
+    if [ $ret -eq 0 ]; then
         echo -e "\e[32mPASS\e[0m $1"
     else
         echo -e "\e[31mFAIL\e[0m $1"
diff --git a/x86/README b/x86/README
index 996ed33546d62..218fe1a1c6f1a 100644
--- a/x86/README
+++ b/x86/README
@@ -41,7 +41,9 @@ Tests in this directory and what they do:
  pcid:		basic functionality test of PCID/INVPCID feature
 
 Legacy notes:
- The exit status of the binary (and the script) is inconsistent: with
- qemu-system, after the unit-test is done, the exit status of qemu is 1,
- different from the 'old style' qemu-kvm, whose exit status in successful
- completion is 0.
+ The exit status of the binary is inconsistent; with qemu-system, after
+ the unit-test is done, the exit status of qemu is 1, different from the
+ 'old style' qemu-kvm, whose exit status in successful completion is 0.
+ The run script converts the qemu-system exit status to 0 (SUCCESS), and
+ treats the legacy exit status of 0 as an error, converting it to an exit
+ status of 1.
diff --git a/x86/run b/x86/run
index 22d7f2cad0d06..b07c815c5a6e1 100755
--- a/x86/run
+++ b/x86/run
@@ -1,5 +1,7 @@
 #!/bin/bash
 
+[ -z "$STANDALONE" ] && source scripts/arch-run.bash
+
 qemubinarysearch="${QEMU:-qemu-kvm qemu-system-x86_64}"
 
 for qemucmd in ${qemubinarysearch}
@@ -43,7 +45,4 @@ fi
 command="${qemu} -enable-kvm $pc_testdev -vnc none -serial stdio $pci_testdev $hyperv_testdev -kernel"
 echo ${command} "$@"
 
-${command} "$@"
-ret=$?
-echo Return value from qemu: $ret
-exit $ret
+exit_fixup ${command} "$@"
-- 
2.4.3

Re: [kvm-unit-tests PATCH v3 3/6] arch-run: reduce return code ambiguity

[Paolo Bonzini]

On 29/02/2016 19:53, Andrew Jones wrote:
> +exit_fixup ()

Can you rename the function to run_qemu or something like that?
Otherwise it's not clear that it actually runs "$@".

> +	errors=$("${@}" 2>&1 1>&${stdout} | tee >(cat - 1>&2); exit ${PIPESTATUS[0]})

Whoa! :)  So stdout goes to {stdout} and from there to the real stdout;
stderr instead is tee'd to stderr and $errors.

Is the "tee" and "cat" necessary?  Can you just use printf somewhat like:

	# stdout to {stdout}, stderr to $errors
	errors=$("$@" 2>&1 1>&${stdout})
	ret=$?
	printf '%s\n' "$errors" >&2

?  Or something like that (for example I'm not sure if you need the \n).

In either case, stdout and stderr can be mixed.

> +	temp_file RUNTIME_arch_run <(echo "#!/bin/bash"; cat scripts/arch-run.bash "$TEST_DIR/run")

Please use a pipe instead of a <(...):

	(echo "#! /bin/bash"
	 cat scripts/arch-run.bash "$TEST_DIR/run") | tempfile RUNTIME_arch_run

by changing this in temp_file:

	local file="$2"
	gzip - < $file

to

	local file="${2--}"
	gzip -c "$file"

Paolo

Re: [kvm-unit-tests PATCH v3 3/6] arch-run: reduce return code ambiguity

[Radim Krčmář]

2016-03-01 22:29+0100, Paolo Bonzini:
> On 29/02/2016 19:53, Andrew Jones wrote:
>> +	errors=$("${@}" 2>&1 1>&${stdout} | tee >(cat - 1>&2); exit ${PIPESTATUS[0]})
> 
> Whoa! :)  So stdout goes to {stdout} and from there to the real stdout;
> stderr instead is tee'd to stderr and $errors.

Yeah. :)

> Is the "tee" and "cat" necessary?  Can you just use printf somewhat like:

They are, if you want want to improve chances of getting the original
order of stdout and stderr.

> 	# stdout to {stdout}, stderr to $errors
> 	errors=$("$@" 2>&1 1>&${stdout})
> 	ret=$?
> 	printf '%s\n' "$errors" >&2

(Previous version did this any my suggestion was to change it ...)

> ?  Or something like that (for example I'm not sure if you need the \n).

Trying to improve the original solution, What about a bit more
understandable

  errors=$("${@}" 2> >(tee >(cat) >&2) >&$stdout)

?

Which gets rid of some redirections and PIPESTATUS.

> In either case, stdout and stderr can be mixed.

Yes, we can't have a good solution here.

>> +	temp_file RUNTIME_arch_run <(echo "#!/bin/bash"; cat scripts/arch-run.bash "$TEST_DIR/run")
> 
> Please use a pipe instead of a <(...):

(Is it for aesthetic reasons?)

Thanks.

Re: [kvm-unit-tests PATCH v3 3/6] arch-run: reduce return code ambiguity

[Paolo Bonzini]

On 02/03/2016 13:57, Radim Krčmář wrote:
> 2016-03-01 22:29+0100, Paolo Bonzini:
>> On 29/02/2016 19:53, Andrew Jones wrote:
>>> +	errors=$("${@}" 2>&1 1>&${stdout} | tee >(cat - 1>&2); exit ${PIPESTATUS[0]})
>>
>> Whoa! :)  So stdout goes to {stdout} and from there to the real stdout;
>> stderr instead is tee'd to stderr and $errors.
> 
> Yeah. :)
> 
>> Is the "tee" and "cat" necessary?  Can you just use printf somewhat like:
> 
> They are, if you want want to improve chances of getting the original
> order of stdout and stderr.

Getting the original order can't really work if you use separate file
descriptions.

I'm not sure how you would have something on stderr, except on the very
last line?  The idea being that 1) the test only writes to stdout 2)
QEMU only writes to stderr 3) QEMU exits after writing to stderr.  If
this is true, the proposed assignment/printf pair works just fine.

>>> +	temp_file RUNTIME_arch_run <(echo "#!/bin/bash"; cat scripts/arch-run.bash "$TEST_DIR/run")
>>
>> Please use a pipe instead of a <(...):
> 
> (Is it for aesthetic reasons?)

Yes, nothing more.  Also, gzip has "-c", might as well use it.

Paolo

Re: [kvm-unit-tests PATCH v3 3/6] arch-run: reduce return code ambiguity

[Radim Krčmář]

2016-03-02 15:44+0100, Paolo Bonzini:
> On 02/03/2016 13:57, Radim Krčmář wrote:
>> 2016-03-01 22:29+0100, Paolo Bonzini:
>>> On 29/02/2016 19:53, Andrew Jones wrote:
>>>> +	errors=$("${@}" 2>&1 1>&${stdout} | tee >(cat - 1>&2); exit ${PIPESTATUS[0]})
>>>
>>> Whoa! :)  So stdout goes to {stdout} and from there to the real stdout;
>>> stderr instead is tee'd to stderr and $errors.
>> 
>> Yeah. :)
>> 
>>> Is the "tee" and "cat" necessary?  Can you just use printf somewhat like:
>> 
>> They are, if you want want to improve chances of getting the original
>> order of stdout and stderr.
> 
> Getting the original order can't really work if you use separate file
> descriptions.

Yes, there is no "true" order ... my concern is about transparency:
tee delays stderr much less than if we buffered stderr until QEMU has
finished, so mixed stdout/stderr has better chance of being visible in
sans-wrapper order.

> I'm not sure how you would have something on stderr, except on the very
> last line?  The idea being that 1) the test only writes to stdout 2)
> QEMU only writes to stderr 3) QEMU exits after writing to stderr.  If
> this is true, the proposed assignment/printf pair works just fine.

QEMU prints some warnings without immediately exiting, e.g.
  warning: host doesn't support requested feature: CPUID.01H:ECX.vmx [bit 5]

So it's possible that stdout is printed after stderr.  Preserving the
visible output could be less confusing in these rare cases.  I agree
with the late printf if you think that the ugly code is not worth it.

Re: [kvm-unit-tests PATCH v3 3/6] arch-run: reduce return code ambiguity

[Paolo Bonzini]

On 02/03/2016 16:42, Radim Krčmář wrote:
>> > I'm not sure how you would have something on stderr, except on the very
>> > last line?  The idea being that 1) the test only writes to stdout 2)
>> > QEMU only writes to stderr 3) QEMU exits after writing to stderr.  If
>> > this is true, the proposed assignment/printf pair works just fine.
> 
> QEMU prints some warnings without immediately exiting, e.g.
>   warning: host doesn't support requested feature: CPUID.01H:ECX.vmx [bit 5]
> 
> So it's possible that stdout is printed after stderr.  Preserving the
> visible output could be less confusing in these rare cases.  I agree
> with the late printf if you think that the ugly code is not worth it.

Perhaps we can add an "stderr:" line if $errors is not empty, so that it
is clear that the order is not maintained.

Paolo

Re: [kvm-unit-tests PATCH v3 3/6] arch-run: reduce return code ambiguity

[Radim Krčmář]

2016-03-02 17:05+0100, Paolo Bonzini:
> On 02/03/2016 16:42, Radim Krčmář wrote:
>>> > I'm not sure how you would have something on stderr, except on the very
>>> > last line?  The idea being that 1) the test only writes to stdout 2)
>>> > QEMU only writes to stderr 3) QEMU exits after writing to stderr.  If
>>> > this is true, the proposed assignment/printf pair works just fine.
>> 
>> QEMU prints some warnings without immediately exiting, e.g.
>>   warning: host doesn't support requested feature: CPUID.01H:ECX.vmx [bit 5]
>> 
>> So it's possible that stdout is printed after stderr.  Preserving the
>> visible output could be less confusing in these rare cases.  I agree
>> with the late printf if you think that the ugly code is not worth it.
> 
> Perhaps we can add an "stderr:" line if $errors is not empty, so that it
> is clear that the order is not maintained.

I prefer to print stderr afterwards without any separator.

("stderr:\n" taints stderr and the information provided by it isn't that
 hard to figure out from the output or code.

 I think that this line would attract attention, because it is an
 unexpected element, and therefore waste more time than silently
 reordering the output.  The line might be useful when we merge stdout
 and stderr into one stream, but that doesn't happen at this point.)

[kvm-unit-tests PATCH v3 4/6] cleanup unittests.cfg headers

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

---
powerpc's unittests.cfg header is already clean.

 arm/unittests.cfg | 26 +++++++++++++++++---------
 x86/unittests.cfg | 21 ++++++++++++++-------
 2 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 5e26da1a8c1bc..926bbb153728b 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -1,13 +1,21 @@
-# Define your new unittest following the convention:
+##############################################################################
+# unittest configuration
+#
 # [unittest_name]
-# file = foo.flat # Name of the flat file to be used
-# smp  = 2        # Number of processors the VM will use during this test
-#                 # Use $MAX_SMP to use the maximum the host supports.
-# extra_params = -append <params...> # Additional parameters used
-# arch = arm|arm64                   # Only if test case is specific to one
-# groups = group1 group2 # Used to identify test cases with run_tests -g ...
-# accel = kvm|tcg # Optionally specify if test must run with kvm or tcg.
-#                 # If not specified, then kvm will be used when available.
+# file = <name>.flat		# Name of the flat file to be used.
+# smp  = <num>			# Number of processors the VM will use
+#				# during this test. Use $MAX_SMP to use
+#				# the maximum the host supports. Defaults
+#				# to one.
+# extra_params = -append <params...>	# Additional parameters used.
+# arch = arm|arm64			# Select one if the test case is
+#					# specific to only one.
+# groups = <group_name1> <group_name2> ...	# Used to identify test cases
+#						# with run_tests -g ...
+# accel = kvm|tcg		# Optionally specify if test must run with
+#				# kvm or tcg. If not specified, then kvm will
+#				# be used when available.
+##############################################################################
 
 #
 # Test that the configured number of processors (smp = <num>), and
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 99eff2662e191..4a9564536e8ea 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -1,11 +1,18 @@
-# Define your new unittest following the convention:
+##############################################################################
+# unittest configuration
+#
 # [unittest_name]
-# file = foo.flat # Name of the flat file to be used
-# smp = 2 # Number of processors the VM will use during this test
-#         # Use $MAX_SMP to use the maximum the host supports.
-# extra_params = -cpu qemu64,+x2apic # Additional parameters used
-# arch = i386/x86_64 # Only if the test case works only on one of them
-# groups = group1 group2 # Used to identify test cases with run_tests -g ...
+# file = <name>.flat		# Name of the flat file to be used.
+# smp  = <num>			# Number of processors the VM will use
+#				# during this test. Use $MAX_SMP to use
+#				# the maximum the host supports. Defaults
+#				# to one.
+# extra_params = -append <params...>	# Additional parameters used.
+# arch = i386|x86_64			# Select one if the test case is
+#					# specific to only one.
+# groups = <group_name1> <group_name2> ...	# Used to identify test cases
+#						# with run_tests -g ...
+##############################################################################
 
 [apic]
 file = apic.flat
-- 
2.4.3

[kvm-unit-tests PATCH v3 5/6] runtime: enable some unittest config overriding

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
---
 scripts/runtime.bash | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 9c973f26f8de6..1ac57ccdfc614 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -10,8 +10,8 @@ function run()
     local kernel="$4"
     local opts="$5"
     local arch="$6"
-    local check="$7"
-    local accel="$8"
+    local check="${CHECK:-$7}"
+    local accel="${ACCEL:-$8}"
 
     if [ -z "$testname" ]; then
         return
@@ -57,7 +57,7 @@ function run()
     return $ret
 }
 
-MAX_SMP=$(getconf _NPROCESSORS_CONF)
+: ${MAX_SMP:=$(getconf _NPROCESSORS_CONF)}
 #
 # Probe for MAX_SMP, in case it's less than the number of host cpus.
 #
-- 
2.4.3

[kvm-unit-tests PATCH v3 6/6] run scripts: add timeout support

[Andrew Jones]

Also remove useless qemu variable in scripts/runtime.bash.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

---
v3: now also powerpc/run. It may be noticed that powerpc/unittests.cfg
    is not getting updated, as x86 and arm are. That's because I
    accidentally already added the timeout variable comment to powerpc's
    unittest.cfg header...

 arm/run                |  1 +
 arm/unittests.cfg      |  1 +
 powerpc/run            |  1 +
 scripts/arch-run.bash  |  8 ++++++++
 scripts/functions.bash |  8 ++++++--
 scripts/runtime.bash   | 10 ++++++----
 x86/run                |  1 +
 x86/unittests.cfg      |  1 +
 8 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/arm/run b/arm/run
index c9463de6dd241..4b9aeb3665436 100755
--- a/arm/run
+++ b/arm/run
@@ -70,6 +70,7 @@ chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
 M+=",accel=$ACCEL"
 command="$qemu $M -cpu $processor $chr_testdev"
 command+=" -display none -serial stdio -kernel"
+command="$(timeout_cmd) $command"
 echo $command "$@"
 
 exit_fixup $command "$@"
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 926bbb153728b..8c6c475f050fc 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -15,6 +15,7 @@
 # accel = kvm|tcg		# Optionally specify if test must run with
 #				# kvm or tcg. If not specified, then kvm will
 #				# be used when available.
+# timeout = <duration>		# Optionally specify a timeout.
 ##############################################################################
 
 #
diff --git a/powerpc/run b/powerpc/run
index 8ac684cc7c12f..b012789d962cd 100755
--- a/powerpc/run
+++ b/powerpc/run
@@ -45,6 +45,7 @@ M='-machine pseries'
 M+=",accel=$ACCEL"
 command="$qemu $M -bios $FIRMWARE"
 command+=" -display none -serial stdio -kernel"
+command="$(timeout_cmd) $command"
 echo $command "$@"
 
 # powerpc tests currently exit with rtas-poweroff, which exits with 0.
diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
index 3a63ed179a7de..28a33f41b6ede 100644
--- a/scripts/arch-run.bash
+++ b/scripts/arch-run.bash
@@ -19,6 +19,7 @@
 # 1      - most likely QEMU failed
 # 2      - most likely a run script failed
 # 3      - most likely the unittest failed
+# 124    - most likely the unittest timed out
 # 127    - most likely the unittest called abort()
 # 1..127 - FAILURE (could be QEMU, a run script, or the unittest)
 # >= 128 - Signal (signum = status - 128)
@@ -59,3 +60,10 @@ exit_fixup ()
 
 	return $ret
 }
+
+timeout_cmd ()
+{
+	if [ "$TIMEOUT" ] && [ "$TIMEOUT" != "0" ]; then
+		echo "timeout -k 1s --foreground $TIMEOUT"
+	fi
+}
diff --git a/scripts/functions.bash b/scripts/functions.bash
index f13fe6f88f23d..ee9143c5d630d 100644
--- a/scripts/functions.bash
+++ b/scripts/functions.bash
@@ -11,12 +11,13 @@ function for_each_unittest()
 	local arch
 	local check
 	local accel
+	local timeout
 
 	exec {fd}<"$unittests"
 
 	while read -u $fd line; do
 		if [[ "$line" =~ ^\[(.*)\]$ ]]; then
-			"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel"
+			"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
 			testname=${BASH_REMATCH[1]}
 			smp=1
 			kernel=""
@@ -25,6 +26,7 @@ function for_each_unittest()
 			arch=""
 			check=""
 			accel=""
+			timeout=""
 		elif [[ $line =~ ^file\ *=\ *(.*)$ ]]; then
 			kernel=$TEST_DIR/${BASH_REMATCH[1]}
 		elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
@@ -39,8 +41,10 @@ function for_each_unittest()
 			check=${BASH_REMATCH[1]}
 		elif [[ $line =~ ^accel\ *=\ *(.*)$ ]]; then
 			accel=${BASH_REMATCH[1]}
+		elif [[ $line =~ ^timeout\ *=\ *(.*)$ ]]; then
+			timeout=${BASH_REMATCH[1]}
 		fi
 	done
-	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel"
+	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
 	exec {fd}<&-
 }
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 1ac57ccdfc614..0e055f0dddc2c 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -1,6 +1,6 @@
 : "${RUNTIME_arch_run?}"
-
-qemu=${QEMU:-qemu-system-$ARCH}
+: ${MAX_SMP:=$(getconf _NPROCESSORS_CONF)}
+: ${TIMEOUT:=90s}
 
 function run()
 {
@@ -12,6 +12,7 @@ function run()
     local arch="$6"
     local check="${CHECK:-$7}"
     local accel="${ACCEL:-$8}"
+    local timeout="${9:-$TIMEOUT}" # unittests.cfg overrides the default
 
     if [ -z "$testname" ]; then
         return
@@ -38,7 +39,7 @@ function run()
         fi
     done
 
-    cmdline="TESTNAME=$testname ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
+    cmdline="TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
     if [ "$verbose" = "yes" ]; then
         echo $cmdline
     fi
@@ -50,6 +51,8 @@ function run()
 
     if [ $ret -eq 0 ]; then
         echo -e "\e[32mPASS\e[0m $1"
+    elif [ $ret -eq 124 ]; then
+        echo -e "\e[31mFAIL\e[0m $1 (timeout; duration=$timeout)"
     else
         echo -e "\e[31mFAIL\e[0m $1"
     fi
@@ -57,7 +60,6 @@ function run()
     return $ret
 }
 
-: ${MAX_SMP:=$(getconf _NPROCESSORS_CONF)}
 #
 # Probe for MAX_SMP, in case it's less than the number of host cpus.
 #
diff --git a/x86/run b/x86/run
index b07c815c5a6e1..3386524957a61 100755
--- a/x86/run
+++ b/x86/run
@@ -43,6 +43,7 @@ else
 fi
 
 command="${qemu} -enable-kvm $pc_testdev -vnc none -serial stdio $pci_testdev $hyperv_testdev -kernel"
+command="$(timeout_cmd) $command"
 echo ${command} "$@"
 
 exit_fixup ${command} "$@"
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 4a9564536e8ea..b6094a4477ad5 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -12,6 +12,7 @@
 #					# specific to only one.
 # groups = <group_name1> <group_name2> ...	# Used to identify test cases
 #						# with run_tests -g ...
+# timeout = <duration>		# Optionally specify a timeout.
 ##############################################################################
 
 [apic]
-- 
2.4.3

Re: [kvm-unit-tests PATCH v3 0/6] reduce exit status ambiguity and more

[Paolo Bonzini]

On 29/02/2016 19:53, Andrew Jones wrote:
> The first three patches makes changes to reduce the exit status ambiguity,
> for which kvm-unit-tests is famous. The next couple are a cleanup and a
> minor feature enhancement. The last is a feature enhancement bringing
> timeouts to unittest runs.
> 
> v3: rebased on latest master; now includes powerpc/run changes (specific
>     differences with v2 are pointed out in the patches that changed -
>     only two of them)
> v2: Thanks to Radim for suggestions and help getting the suggestions
>     implemented.

Just a few suggested cleanups in patch 3.

Thanks!

Paolo