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

[kvm-unit-tests PATCH 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.


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 +--
 run_tests.sh              |  4 +--
 scripts/arch-run.bash     | 70 +++++++++++++++++++++++++++++++++++++++++++++++
 scripts/functions.bash    |  8 ++++--
 scripts/mkstandalone.sh   | 20 ++++++--------
 scripts/runtime.bash      | 19 ++++++++-----
 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 +-
 16 files changed, 159 insertions(+), 69 deletions(-)
 create mode 100644 scripts/arch-run.bash

-- 
2.4.3

[kvm-unit-tests PATCH 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>
---
 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 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>
---
 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 558b8e7431d8e..5b0aec1c599c0 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 b0f1e7c098afb..a8781b0c9835f 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"
@@ -68,11 +68,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
@@ -81,8 +81,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 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>
---
 arm/run                 |  6 ++---
 scripts/arch-run.bash   | 62 +++++++++++++++++++++++++++++++++++++++++++++++++
 scripts/mkstandalone.sh |  6 ++---
 scripts/runtime.bash    |  2 +-
 x86/README              | 10 ++++----
 x86/run                 |  7 +++---
 6 files changed, 77 insertions(+), 16 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/scripts/arch-run.bash b/scripts/arch-run.bash
new file mode 100644
index 0000000000000..2701f8fb220e8
--- /dev/null
+++ b/scripts/arch-run.bash
@@ -0,0 +1,62 @@
+##############################################################################
+# 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 nr_errors ret sig
+
+	exec {stdout}>&1
+	mapfile -t errors < <("${@}" 2>&1 1>&${stdout}; echo $?)
+	exec {stdout}>&-
+
+	nr_errors=$((${#errors[@]} - 1))
+	ret=${errors[$nr_errors]}
+	unset errors[$nr_errors]
+
+	if [ $nr_errors -ne 0 ]; then
+		printf "%s\n" "${errors[@]}"
+		sig=$(sed 's/.*terminating on signal \([0-9][0-9]*\).*/\1/' <<<"${errors[@]}")
+	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 [ $nr_errors -eq 0 ]; then
+			ret=0
+		fi
+	fi
+
+	return $ret
+}
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index a8781b0c9835f..4b96a849063d1 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -17,13 +17,13 @@ escape ()
 temp_file ()
 {
 	local var="$1"
-	local file="$2"
+	shift
 
 	echo "$var=\`mktemp\`"
 	echo "cleanup=\"\$$var \$cleanup\""
 	echo "base64 -d << 'BIN_EOF' | zcat > \$$var || exit 2"
 
-	gzip - < $file | base64
+	gzip -c "$@" | base64
 
 	echo "BIN_EOF"
 	echo "chmod +x \$$var"
@@ -56,7 +56,7 @@ generate_test ()
 	temp_file bin "$kernel"
 	args[3]='$bin'
 
-	temp_file RUNTIME_arch_run "$TEST_DIR/run"
+	temp_file RUNTIME_arch_run 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

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

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 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 5/6] runtime: enable some unittest config overriding

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@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..c22a6a7883dec 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=${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 6/6] run scripts: add timeout support

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arm/run                | 1 +
 arm/unittests.cfg      | 1 +
 scripts/arch-run.bash  | 8 ++++++++
 scripts/functions.bash | 8 ++++++--
 scripts/runtime.bash   | 7 ++++++-
 x86/run                | 1 +
 x86/unittests.cfg      | 1 +
 7 files changed, 24 insertions(+), 3 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/scripts/arch-run.bash b/scripts/arch-run.bash
index 2701f8fb220e8..40c0a460c765f 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)
@@ -60,3 +61,10 @@ exit_fixup ()
 
 	return $ret
 }
+
+timeout_cmd ()
+{
+	if [ "$TIMEOUT" ] && [ "$TIMEOUT" != "0" ]; then
+		echo "timeout --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 c22a6a7883dec..1414bd4f22d32 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -1,5 +1,7 @@
 : "${RUNTIME_arch_run?}"
 
+TIMEOUT=${TIMEOUT:-90s}
+
 qemu=${QEMU:-qemu-system-$ARCH}
 
 function run()
@@ -12,6 +14,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 +41,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 +53,8 @@ function run()
 
     if [ $ret -eq 0 ]; then
         echo -e "\e[32mPASS\e[0m $1"
+    elif [ $ret -eq 124 ]; then
+        echo -e "\e[32mFAIL\e[0m $1 (timeout; duration=$timeout)"
     else
         echo -e "\e[31mFAIL\e[0m $1"
     fi
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 3/6] arch-run: reduce return code ambiguity

[Radim Krčmář]

2016-01-26 19:42+0100, 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>
> ---
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> @@ -0,0 +1,62 @@
> +##############################################################################
> +# 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 nr_errors ret sig
> +
> +	exec {stdout}>&1
> +	mapfile -t errors < <("${@}" 2>&1 1>&${stdout}; echo $?)
> +	exec {stdout}>&-
> +
> +	nr_errors=$((${#errors[@]} - 1))
> +	ret=${errors[$nr_errors]}
> +	unset errors[$nr_errors]

I think would be easier to understand as
  errors=$("${@}" 2>&1 1>&${stdout})
  ret=$?
  # using [ "$errors" ] instead of [ $nr_errors -ne 0 ]

> +
> +	if [ $nr_errors -ne 0 ]; then
> +		printf "%s\n" "${errors[@]}"

Should go to stderr.

(This reorders the output, so it would be a better to duplicate it when
 storing and print right away, but I only see inhumane designs for that.)

> +		sig=$(sed 's/.*terminating on signal \([0-9][0-9]*\).*/\1/' <<<"${errors[@]}")

If ${errors[@]} don't contain just 'terminating [...]' and QEMU returned
0, some text will be assigned to sig, which isn't going to be handled
well later on.

> +	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 [ $nr_errors -eq 0 ]; then
> +			ret=0
> +		fi
> +	fi
> +
> +	return $ret
> +}

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

[Radim Krčmář]

2016-01-26 19:42+0100, Andrew Jones:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> @@ -57,7 +57,7 @@ function run()
> -MAX_SMP=$(getconf _NPROCESSORS_CONF)
> +MAX_SMP=${MAX_SMP:-$(getconf _NPROCESSORS_CONF)}

Shell can save characters even on that ...
   : ${MAX_SMP:=$(getconf _NPROCESSORS_CONF)}

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

[Radim Krčmář]

2016-01-26 19:42+0100, Andrew Jones:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash
> @@ -60,3 +61,10 @@ exit_fixup ()
>  
>  	return $ret
>  }
> +
> +timeout_cmd ()
> +{
> +	if [ "$TIMEOUT" ] && [ "$TIMEOUT" != "0" ]; then
> +		echo "timeout --foreground $TIMEOUT"

We could add "-k  $(($TIMEOUT + 5))" in case QEMU ignores SIGTERM ...
or just directly SIGKILL?

> +	fi
> +}

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

[Radim Krčmář]

2016-01-26 19:42+0100, 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.

I can accept that we don't care about bugs in [3/6], so if you insist,
Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

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

[Andrew Jones]

On Tue, Jan 26, 2016 at 10:55:34PM +0100, Radim Krčmář wrote:
> 2016-01-26 19:42+0100, 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.
> 
> I can accept that we don't care about bugs in [3/6], so if you insist,

I never insist. I'll fix it up with your suggestions. I also thought
of something else to fix. The standalone arch-run script is now the
concatenation of scripts/arch-run.bash and TEST_DIR/run, the former being
first. scripts/arch-run.bash doesn't have a '#!/bin/bash' at the top.
I'll modify mkstandalone to make sure RUNTIME_arch_run always has bash
specified.

> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>

I'll add your r-b's to all patches for v2. If you don't like something I
do for the changes then just complain and revoke it :-)

Thanks,
drew

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

[Radim Krčmář]

2016-01-27 11:33+0100, Andrew Jones:
> On Tue, Jan 26, 2016 at 10:55:34PM +0100, Radim Krčmář wrote:
>> 2016-01-26 19:42+0100, 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.
>> 
>> I can accept that we don't care about bugs in [3/6], so if you insist,
> 
> I never insist. I'll fix it up with your suggestions. I also thought
> of something else to fix. The standalone arch-run script is now the
> concatenation of scripts/arch-run.bash and TEST_DIR/run, the former being
> first. scripts/arch-run.bash doesn't have a '#!/bin/bash' at the top.
> I'll modify mkstandalone to make sure RUNTIME_arch_run always has bash
> specified.

True.  The sad part is that it worked in tests, because sh = bash on
many systems ...

>> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com>
> 
> I'll add your r-b's to all patches for v2. If you don't like something I
> do for the changes then just complain and revoke it :-)

Thanks.