[PATCH kvm-unit-tests v2 00/12] Improve the output of test runners

[PATCH kvm-unit-tests v2 00/12] Improve the output of test runners

[Radim Krčmář]

v1: http://www.spinics.net/lists/kvm/msg125202.html

Drew brought up the existence of scripts/mkstandalone.sh, which
significantly expanded v2 (and my set of curses) ...
I didn't want to do the same twice, so first part of this series,
[1-4/12], reuses run() from run_tests.sh and does some non-conservative
changes to scripts/mkstandalone.sh.  scripts/mkstandalone.sh is lacking
behind run_tests.sh, but should be good enough to fulfill its purpose.

The output of run_tests.sh has also changed a bit and now looks like
this (you'll again need to imagine colours):

> PASS apic (14 tests)
> PASS ioapic (19 tests)
> PASS smptest (1 tests)
> PASS smptest3 (1 tests)
> PASS vmexit_cpuid
> PASS vmexit_vmcall
> PASS vmexit_mov_from_cr8
> PASS vmexit_mov_to_cr8
> PASS vmexit_inl_pmtimer
> PASS vmexit_ipi
> PASS vmexit_ipi_halt
> PASS vmexit_ple_round_robin
> PASS access
> SKIP smap (0 tests)
> SKIP pku (0 tests)
> PASS emulator (132 tests, 1 skipped)
> PASS eventinj (13 tests)
> PASS hypercall (2 tests)
> PASS idt_test (4 tests)
> PASS msr (13 tests)
> PASS pmu (67 tests, 1 expected failures)
> PASS port80
> PASS realmode
> PASS s3
> PASS sieve
> PASS tsc (3 tests)
> PASS tsc_adjust (5 tests)
> PASS xsave (17 tests)
> PASS rmap_chain
> SKIP svm (0 tests)
> SKIP svm-disabled (0 tests)
> SKIP taskswitch (i386 only)
> SKIP taskswitch2 (i386 only)
> PASS kvmclock_test
> PASS pcid (3 tests)
> SKIP vmx (0 tests)
> PASS debug (7 tests)
> SKIP hyperv_synic (failed check)


Radim Krčmář (12):
  run_tests: move run() to scripts/
  run_tests: prepare for changes in scripts/mkstandalone
  scripts/mkstandalone: use common run function
  scripts/mkstandalone: improve exit paths
  lib/report: allow test skipping
  x86/*: report skipped tests
  x86/pmu: expect failure with nmi_watchdog
  scripts/run: generalize check
  x86/hyperv_synic: check for support before testing
  run_tests: print summary
  wrappers: consolidate skip output
  run_tests: suppress stderr

 lib/libcflat.h          |  1 +
 lib/report.c            | 44 +++++++++++++++++++++++-----------
 run_tests.sh            | 58 +++++---------------------------------------
 scripts/mkstandalone.sh | 64 +++++++++++++++++++++----------------------------
 scripts/run.bash        | 62 +++++++++++++++++++++++++++++++++++++++++++++++
 x86/apic.c              |  7 +++---
 x86/emulator.c          |  2 +-
 x86/hyperv_synic.c      |  2 +-
 x86/pku.c               |  2 +-
 x86/pmu.c               | 11 +++++++--
 x86/smap.c              |  2 +-
 x86/svm.c               |  2 +-
 x86/tsc.c               |  2 +-
 x86/unittests.cfg       |  4 ++--
 14 files changed, 146 insertions(+), 117 deletions(-)
 create mode 100644 scripts/run.bash

-- 
2.6.4

[PATCH kvm-unit-tests v2 01/12] run_tests: move run() to scripts/

[Radim Krčmář]

We'll be using it from scripts/mkstandalone later.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: new
 
 run_tests.sh     | 53 +----------------------------------------------------
 scripts/run.bash | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 52 deletions(-)
 create mode 100644 scripts/run.bash

diff --git a/run_tests.sh b/run_tests.sh
index fad22a935b00..58949e39c38c 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -6,63 +6,12 @@ if [ ! -f config.mak ]; then
 fi
 source config.mak
 source scripts/functions.bash
+source scripts/run.bash
 
 config=$TEST_DIR/unittests.cfg
 qemu=${QEMU:-qemu-system-$ARCH}
 verbose=0
 
-function run()
-{
-    local testname="$1"
-    local groups="$2"
-    local smp="$3"
-    local kernel="$4"
-    local opts="$5"
-    local arch="$6"
-    local check="$7"
-    local accel="$8"
-
-    if [ -z "$testname" ]; then
-        return
-    fi
-
-    if [ -n "$only_group" ] && ! grep -q "$only_group" <<<$groups; then
-        return
-    fi
-
-    if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
-        echo "skip $1 ($arch only)"
-        return
-    fi
-
-    # check a file for a particular value before running a test
-    # the check line can contain multiple files to check separated by a space
-    # but each check parameter needs to be of the form <path>=<value>
-    for check_param in ${check[@]}; do
-        path=${check_param%%=*}
-        value=${check_param#*=}
-        if [ "$path" ] && [ "$(cat $path)" != "$value" ]; then
-            echo "skip $1 ($path not equal to $value)"
-            return
-        fi
-    done
-
-    cmdline="TESTNAME=$testname ACCEL=$accel ./$TEST_DIR-run $kernel -smp $smp $opts"
-    if [ $verbose != 0 ]; then
-        echo $cmdline
-    fi
-
-    # extra_params in the config file may contain backticks that need to be
-    # expanded, so use eval to start qemu
-    eval $cmdline >> test.log
-
-    if [ $? -le 1 ]; then
-        echo -e "\e[32mPASS\e[0m $1"
-    else
-        echo -e "\e[31mFAIL\e[0m $1"
-    fi
-}
-
 function usage()
 {
 cat <<EOF
diff --git a/scripts/run.bash b/scripts/run.bash
new file mode 100644
index 000000000000..0c5a2569d80e
--- /dev/null
+++ b/scripts/run.bash
@@ -0,0 +1,51 @@
+function run()
+{
+    local testname="$1"
+    local groups="$2"
+    local smp="$3"
+    local kernel="$4"
+    local opts="$5"
+    local arch="$6"
+    local check="$7"
+    local accel="$8"
+
+    if [ -z "$testname" ]; then
+        return
+    fi
+
+    if [ -n "$only_group" ] && ! grep -q "$only_group" <<<$groups; then
+        return
+    fi
+
+    if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
+        echo "skip $1 ($arch only)"
+        return
+    fi
+
+    # check a file for a particular value before running a test
+    # the check line can contain multiple files to check separated by a space
+    # but each check parameter needs to be of the form <path>=<value>
+    for check_param in ${check[@]}; do
+        path=${check_param%%=*}
+        value=${check_param#*=}
+        if [ "$path" ] && [ "$(cat $path)" != "$value" ]; then
+            echo "skip $1 ($path not equal to $value)"
+            return
+        fi
+    done
+
+    cmdline="TESTNAME=$testname ACCEL=$accel ./$TEST_DIR-run $kernel -smp $smp $opts"
+    if [ $verbose != 0 ]; then
+        echo $cmdline
+    fi
+
+    # extra_params in the config file may contain backticks that need to be
+    # expanded, so use eval to start qemu
+    eval $cmdline >> test.log
+
+    if [ $? -le 1 ]; then
+        echo -e "\e[32mPASS\e[0m $1"
+    else
+        echo -e "\e[31mFAIL\e[0m $1"
+    fi
+}
-- 
2.6.4

[PATCH kvm-unit-tests v2 02/12] run_tests: prepare for changes in scripts/mkstandalone

[Radim Krčmář]

mkstandalone has a different mechanism for running tests as well as a
different handling of output and return codes.
 - create two shell function to capture test execution and logging
 - return the return value of unit-test
 - cope with empty $verbose in `run`

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: new (reused the bitshift and comment from v1)
 
 run_tests.sh     |  4 ++++
 scripts/run.bash | 13 +++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/run_tests.sh b/run_tests.sh
index 58949e39c38c..e09d410beaa4 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -28,7 +28,11 @@ specify the appropriate qemu binary for ARCH-run.
 EOF
 }
 
+__run() { ./$TEST_DIR-run "${@}"; }
+__eval_log() { eval "${@}" >> test.log; }
+
 echo > test.log
+
 while getopts "g:hv" opt; do
     case $opt in
         g)
diff --git a/scripts/run.bash b/scripts/run.bash
index 0c5a2569d80e..243586c6d2fc 100644
--- a/scripts/run.bash
+++ b/scripts/run.bash
@@ -34,18 +34,23 @@ function run()
         fi
     done
 
-    cmdline="TESTNAME=$testname ACCEL=$accel ./$TEST_DIR-run $kernel -smp $smp $opts"
-    if [ $verbose != 0 ]; then
+    cmdline="TESTNAME=$testname ACCEL=$accel __run $kernel -smp $smp $opts"
+    if [ "$verbose" -a "$verbose" != 0 ]; then
         echo $cmdline
     fi
 
     # extra_params in the config file may contain backticks that need to be
     # expanded, so use eval to start qemu
-    eval $cmdline >> test.log
+    __eval_log "$cmdline"
+    # The first bit of return value is too hard to use, just skip it.
+    # Unit-tests' return value is shifted by one.
+    ret=$(($? >> 1))
 
-    if [ $? -le 1 ]; then
+    if [ $ret -eq 0 ]; then
         echo -e "\e[32mPASS\e[0m $1"
     else
         echo -e "\e[31mFAIL\e[0m $1"
     fi
+
+    return $ret
 }
-- 
2.6.4

[PATCH kvm-unit-tests v2 03/12] scripts/mkstandalone: use common run function

[Radim Krčmář]

The biggest change is dependency on bash.  An alternative would be to
rewrite `run` in POSIX shell, but I think it's ok to presume that KVM
unit tests will run on a system where installing bash isn't a problem.
(We already depend on QEMU ...)

Apart from that, there are changes in output and exit codes.
 - summary doesn't go to stderr
 - PASS/FAIL is colourful
 - FAILed scripts return 1

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: new (I can fix the stderr in v3)
 
 scripts/mkstandalone.sh | 59 +++++++++++++++++++++----------------------------
 1 file changed, 25 insertions(+), 34 deletions(-)

diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index 3ce244aff67b..cf2182dbd936 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -20,6 +20,13 @@ fi
 unittests=$TEST_DIR/unittests.cfg
 mkdir -p tests
 
+escape ()
+{
+	for arg in "${@}"; do
+		printf "%q " "$arg"; # XXX: trailing whitespace
+	done
+}
+
 function mkstandalone()
 {
 	local testname="$1"
@@ -49,33 +56,14 @@ function mkstandalone()
 	cmdline=$(cut -d' ' -f2- <<< "$cmdline")
 
 	cat <<EOF > $standalone
-#!/bin/sh
+#!/bin/bash
 
-EOF
-if [ "$arch" ]; then
-	cat <<EOF >> $standalone
 ARCH=\`uname -m | sed -e s/i.86/i386/ | sed -e 's/arm.*/arm/'\`
-[ "\$ARCH" = "aarch64" ] && ARCH="arm64"
-if [ "\$ARCH" != "$arch" ]; then
-	echo "skip $testname ($arch only)" 1>&2
-	exit 1
-fi
 
 EOF
-fi
-if [ "$check" ]; then
-	cat <<EOF >> $standalone
-for param in $check; do
-	path=\`echo \$param | cut -d= -f1\`
-	value=\`echo \$param | cut -d= -f2\`
-	if [ -f "\$path" ] && [ "\`cat \$path\`" != "\$value" ]; then
-		echo "skip $testname (\$path not equal to \$value)" 1>&2
-		exit 1
-	fi
-done
 
-EOF
-fi
+cat scripts/run.bash >> $standalone
+
 if [ ! -f $kernel ]; then
 	cat <<EOF >> $standalone
 echo "skip $testname (test kernel not present)" 1>&2
@@ -100,9 +88,13 @@ MAX_SMP="MAX_SMP"
 echo \$qemu $cmdline -smp $smp $opts
 
 cmdline="\`echo '$cmdline' | sed s%$kernel%_NO_FILE_4Uhere_%\`"
-if \$qemu \$cmdline 2>&1 | grep 'No accelerator found'; then
-	ret=2
-else
+if \$qemu \$cmdline 2>&1 | grep 'No accelerator found' >/dev/null; then
+	echo "skip $testname (QEMU doesn't support KVM)"
+	exit 1
+fi
+
+__run()
+{
 	MAX_SMP=\`getconf _NPROCESSORS_CONF\`
 	while \$qemu \$cmdline -smp \$MAX_SMP 2>&1 | grep 'exceeds max cpus' > /dev/null; do
 		MAX_SMP=\`expr \$MAX_SMP - 1\`
@@ -110,16 +102,15 @@ else
 
 	cmdline="\`echo '$cmdline' | sed s%$kernel%\$bin%\`"
 	\$qemu \$cmdline -smp $smp $opts
-	ret=\$?
-fi
-echo Return value from qemu: \$ret
-if [ \$ret -le 1 ]; then
-	echo PASS $testname 1>&2
-else
-	echo FAIL $testname 1>&2
-fi
+}
+
+__eval_log() { eval "\${@}"; }
+
+run `escape "${@}"`
+ret=$?
+
 rm -f \$bin
-exit 0
+exit \$ret
 EOF
 fi
 chmod +x $standalone
-- 
2.6.4

[PATCH kvm-unit-tests v2 04/12] scripts/mkstandalone: improve exit paths

[Radim Krčmář]

trap can be called on EXIT, which covers most exits.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: new
 
 scripts/mkstandalone.sh | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index cf2182dbd936..778383077769 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -71,7 +71,7 @@ exit 1
 EOF
 else
 	cat <<EOF >> $standalone
-trap 'rm -f \$bin; exit 1' HUP INT TERM
+trap 'rm -f \$bin' EXIT
 bin=\`mktemp\`
 base64 -d << 'BIN_EOF' | zcat > \$bin &&
 EOF
@@ -107,10 +107,7 @@ __run()
 __eval_log() { eval "\${@}"; }
 
 run `escape "${@}"`
-ret=$?
-
-rm -f \$bin
-exit \$ret
+exit \$?
 EOF
 fi
 chmod +x $standalone
-- 
2.6.4

[PATCH kvm-unit-tests v2 05/12] lib/report: allow test skipping

[Radim Krčmář]

We can now explicitly mark a unit-test as skipped.
If all unit-tests were skipped, the whole test is reported as skipped as
well.  This also includes the case where no tests were run, but still
ended with report_summary().

When the whole test is skipped, ./run_tests.sh prints yellow "SKIP"
instead of green "PASS".

Return value of 77 is used to please Autotools.  I also renamed few
things in reporting code and chose to refactor a logic while at it.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2:
 - turned skip into yellow SKIP [Drew]
 - wrapped line at 80 characters [Drew]
 - added static to va_report
 
 lib/libcflat.h   |  1 +
 lib/report.c     | 44 ++++++++++++++++++++++++++++++--------------
 scripts/run.bash | 12 +++++++-----
 3 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/lib/libcflat.h b/lib/libcflat.h
index 9747ccdbc9f1..070818354ee1 100644
--- a/lib/libcflat.h
+++ b/lib/libcflat.h
@@ -61,6 +61,7 @@ void report_prefix_push(const char *prefix);
 void report_prefix_pop(void);
 void report(const char *msg_fmt, bool pass, ...);
 void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...);
+void report_skip(const char *msg_fmt, ...);
 int report_summary(void);
 
 #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0]))
diff --git a/lib/report.c b/lib/report.c
index 35e664108a92..a47f2e00b529 100644
--- a/lib/report.c
+++ b/lib/report.c
@@ -13,7 +13,7 @@
 #include "libcflat.h"
 #include "asm/spinlock.h"
 
-static unsigned int tests, failures, xfailures;
+static unsigned int tests, failures, xfailures, skipped;
 static char prefixes[256];
 static struct spinlock lock;
 
@@ -43,25 +43,28 @@ void report_prefix_pop(void)
 	spin_unlock(&lock);
 }
 
-void va_report_xfail(const char *msg_fmt, bool xfail, bool cond, va_list va)
+static void va_report(const char *msg_fmt, bool pass, bool xfail, bool skip,
+		va_list va)
 {
-	char *pass = xfail ? "XPASS" : "PASS";
-	char *fail = xfail ? "XFAIL" : "FAIL";
 	char buf[2000];
+	char *prefix = skip ? "SKIP"
+	                    : xfail ? (pass ? "XPASS" : "XFAIL")
+	                            : (pass ? "PASS"  : "FAIL");
 
 	spin_lock(&lock);
 
 	tests++;
-	printf("%s: ", cond ? pass : fail);
+	printf("%s: ", prefix);
 	puts(prefixes);
 	vsnprintf(buf, sizeof(buf), msg_fmt, va);
 	puts(buf);
 	puts("\n");
-	if (xfail && cond)
-		failures++;
-	else if (xfail)
+
+	if (skip)
+		skipped++;
+	else if (xfail && !pass)
 		xfailures++;
-	else if (!cond)
+	else if (xfail || !pass)
 		failures++;
 
 	spin_unlock(&lock);
@@ -71,7 +74,7 @@ void report(const char *msg_fmt, bool pass, ...)
 {
 	va_list va;
 	va_start(va, pass);
-	va_report_xfail(msg_fmt, false, pass, va);
+	va_report(msg_fmt, pass, false, false, va);
 	va_end(va);
 }
 
@@ -79,7 +82,15 @@ void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...)
 {
 	va_list va;
 	va_start(va, pass);
-	va_report_xfail(msg_fmt, xfail, pass, va);
+	va_report(msg_fmt, pass, xfail, false, va);
+	va_end(va);
+}
+
+void report_skip(const char *msg_fmt, ...)
+{
+	va_list va;
+	va_start(va, msg_fmt);
+	va_report(msg_fmt, false, false, true, va);
 	va_end(va);
 }
 
@@ -89,9 +100,14 @@ int report_summary(void)
 
 	printf("\nSUMMARY: %d tests, %d unexpected failures", tests, failures);
 	if (xfailures)
-		printf(", %d expected failures\n", xfailures);
-	else
-		printf("\n");
+		printf(", %d expected failures", xfailures);
+	if (skipped)
+		printf(", %d skipped", skipped);
+	printf("\n");
+
+	if (tests == skipped)
+		return 77; /* blame AUTOTOOLS */
+
 	return failures > 0 ? 1 : 0;
 
 	spin_unlock(&lock);
diff --git a/scripts/run.bash b/scripts/run.bash
index 243586c6d2fc..b92611c29fbb 100644
--- a/scripts/run.bash
+++ b/scripts/run.bash
@@ -46,11 +46,13 @@ function run()
     # Unit-tests' return value is shifted by one.
     ret=$(($? >> 1))
 
-    if [ $ret -eq 0 ]; then
-        echo -e "\e[32mPASS\e[0m $1"
-    else
-        echo -e "\e[31mFAIL\e[0m $1"
-    fi
+    case $ret in
+    0)  echo -ne "\e[32mPASS\e[0m" ;;
+    77) echo -ne "\e[33mSKIP\e[0m" ;;
+    *)  echo -ne "\e[31mFAIL\e[0m"
+    esac
+
+    echo " $1"
 
     return $ret
 }
-- 
2.6.4

[PATCH kvm-unit-tests v2 06/12] x86/*: report skipped tests

[Radim Krčmář]

No care to consistency or exhaustivity was given.

(svm-disabled test should be redone and it's weird that x86/hyperv_synic
 is about the only one that does report_skip when unsupported.)

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: remove double newline in enable_tsc_deadline_timer [Drew]
 
 x86/apic.c         | 7 +++----
 x86/emulator.c     | 2 +-
 x86/hyperv_synic.c | 2 +-
 x86/pku.c          | 2 +-
 x86/pmu.c          | 2 +-
 x86/smap.c         | 2 +-
 x86/svm.c          | 2 +-
 x86/tsc.c          | 2 +-
 8 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/x86/apic.c b/x86/apic.c
index d4eec529e535..a97fe5e5a6c5 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -27,7 +27,7 @@ static void tsc_deadline_timer_isr(isr_regs_t *regs)
     ++tdt_count;
 }
 
-static void start_tsc_deadline_timer(void)
+static void __test_tsc_deadline_timer(void)
 {
     handle_irq(TSC_DEADLINE_TIMER_VECTOR, tsc_deadline_timer_isr);
     irq_enable();
@@ -45,7 +45,6 @@ static int enable_tsc_deadline_timer(void)
     if (cpuid(1).c & (1 << 24)) {
         lvtt = TSC_DEADLINE_TIMER_MODE | TSC_DEADLINE_TIMER_VECTOR;
         apic_write(APIC_LVTT, lvtt);
-        start_tsc_deadline_timer();
         return 1;
     } else {
         return 0;
@@ -55,9 +54,9 @@ static int enable_tsc_deadline_timer(void)
 static void test_tsc_deadline_timer(void)
 {
     if(enable_tsc_deadline_timer()) {
-        printf("tsc deadline timer enabled\n");
+        __test_tsc_deadline_timer();
     } else {
-        printf("tsc deadline timer not detected\n");
+        report_skip("tsc deadline timer not detected");
     }
 }
 
diff --git a/x86/emulator.c b/x86/emulator.c
index e5c1c6b9a2f3..b64a5fe0f3dc 100644
--- a/x86/emulator.c
+++ b/x86/emulator.c
@@ -1062,7 +1062,7 @@ static void illegal_movbe_handler(struct ex_regs *regs)
 static void test_illegal_movbe(void)
 {
 	if (!(cpuid(1).c & (1 << 22))) {
-		printf("SKIP: illegal movbe\n");
+		report_skip("illegal movbe");
 		return;
 	}
 
diff --git a/x86/hyperv_synic.c b/x86/hyperv_synic.c
index 18d1295bfb37..602b79392bfd 100644
--- a/x86/hyperv_synic.c
+++ b/x86/hyperv_synic.c
@@ -228,7 +228,7 @@ int main(int ac, char **av)
 
         report("Hyper-V SynIC test", ok);
     } else {
-        report("Hyper-V SynIC is not supported", true);
+        report_skip("Hyper-V SynIC is not supported");
     }
 
     return report_summary();
diff --git a/x86/pku.c b/x86/pku.c
index 0e00b9984d70..58971d21ed05 100644
--- a/x86/pku.c
+++ b/x86/pku.c
@@ -91,7 +91,7 @@ int main(int ac, char **av)
 
     if (!(cpuid_indexed(7, 0).c & (1 << X86_FEATURE_PKU))) {
         printf("PKU not enabled, exiting\n");
-        exit(1);
+        return report_summary();
     }
 
     setup_vm();
diff --git a/x86/pmu.c b/x86/pmu.c
index 03f80190bb25..c68980044dee 100644
--- a/x86/pmu.c
+++ b/x86/pmu.c
@@ -387,7 +387,7 @@ int main(int ac, char **av)
 
 	if (!eax.split.version_id) {
 		printf("No pmu is detected!\n");
-		return 1;
+		return report_summary();
 	}
 	printf("PMU version:         %d\n", eax.split.version_id);
 	printf("GP counters:         %d\n", eax.split.num_counters);
diff --git a/x86/smap.c b/x86/smap.c
index d8a7ae82dc00..0aa44054bd48 100644
--- a/x86/smap.c
+++ b/x86/smap.c
@@ -93,7 +93,7 @@ int main(int ac, char **av)
 
 	if (!(cpuid_indexed(7, 0).b & (1 << X86_FEATURE_SMAP))) {
 		printf("SMAP not enabled, exiting\n");
-		exit(1);
+		return report_summary();
 	}
 
 	setup_vm();
diff --git a/x86/svm.c b/x86/svm.c
index 1046ddf73732..ff1a0f34b4bf 100644
--- a/x86/svm.c
+++ b/x86/svm.c
@@ -1064,7 +1064,7 @@ int main(int ac, char **av)
 
     if (!(cpuid(0x80000001).c & 4)) {
         printf("SVM not availble\n");
-        return 0;
+        return report_summary();
     }
 
     setup_svm();
diff --git a/x86/tsc.c b/x86/tsc.c
index c71dc2a7abe0..ee247459fb42 100644
--- a/x86/tsc.c
+++ b/x86/tsc.c
@@ -43,5 +43,5 @@ int main()
 		test_rdtscp(0x100);
 	} else
 		printf("rdtscp not supported\n");
-	return 0;
+	return report_summary();
 }
-- 
2.6.4

[PATCH kvm-unit-tests v2 07/12] x86/pmu: expect failure with nmi_watchdog

[Radim Krčmář]

Host's nmi_watchdog takes one slot, making the "all counters" unit-test
fail.  We know exactly what happens, mark it as expected failure.

PMU test is now executed regardless of host_nmi_watchdog.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2:
 - host_nmi_watchdog made static
 
 x86/pmu.c         | 9 ++++++++-
 x86/unittests.cfg | 3 +--
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/x86/pmu.c b/x86/pmu.c
index c68980044dee..70e9b3a41e96 100644
--- a/x86/pmu.c
+++ b/x86/pmu.c
@@ -92,6 +92,7 @@ struct pmu_event {
 };
 
 static int num_counters;
+static bool host_nmi_watchdog;
 
 char *buf;
 
@@ -291,7 +292,7 @@ static void check_counters_many(void)
 		if (!verify_counter(&cnt[i]))
 			break;
 
-	report("all counters", i == n);
+	report_xfail("all counters", host_nmi_watchdog, i == n);
 }
 
 static void check_counter_overflow(void)
@@ -374,6 +375,7 @@ static void check_rdpmc(void)
 
 int main(int ac, char **av)
 {
+	int i;
 	struct cpuid id = cpuid(10);
 
 	setup_vm();
@@ -385,6 +387,11 @@ int main(int ac, char **av)
 	ebx.full = id.b;
 	edx.full = id.d;
 
+	/* XXX: horrible command line parsing */
+	for (i = 1; i < ac; i++)
+		if (!strcmp(av[i], "host_nmi_watchdog=1"))
+			host_nmi_watchdog = true;
+
 	if (!eax.split.version_id) {
 		printf("No pmu is detected!\n");
 		return report_summary();
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index ffffc15c86df..6b94ad93dcf0 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -106,8 +106,7 @@ file = msr.flat
 
 [pmu]
 file = pmu.flat
-extra_params = -cpu host
-check = /proc/sys/kernel/nmi_watchdog=0
+extra_params = -cpu host -append "host_nmi_watchdog=`cat /proc/sys/kernel/nmi_watchdog`"
 
 [port80]
 file = port80.flat
-- 
2.6.4

[PATCH kvm-unit-tests v2 08/12] scripts/run: generalize check

[Radim Krčmář]

config attribute "check" is currently unused.
Provide a simple implementation instead of removing it.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2:
 - update scripts/mkstandalone.sh [Drew]
 - don't print too much [Drew]
 - log the output of check command
 - use $testname
 
 scripts/run.bash | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/scripts/run.bash b/scripts/run.bash
index b92611c29fbb..f532cb9e8b1c 100644
--- a/scripts/run.bash
+++ b/scripts/run.bash
@@ -22,17 +22,11 @@ function run()
         return
     fi
 
-    # check a file for a particular value before running a test
-    # the check line can contain multiple files to check separated by a space
-    # but each check parameter needs to be of the form <path>=<value>
-    for check_param in ${check[@]}; do
-        path=${check_param%%=*}
-        value=${check_param#*=}
-        if [ "$path" ] && [ "$(cat $path)" != "$value" ]; then
-            echo "skip $1 ($path not equal to $value)"
-            return
-        fi
-    done
+    __eval_log "$check" || {
+        __eval_log 'echo "skipped $testname (check returned $?)"'
+        echo "skip $testname (failed check)"
+        return
+    }
 
     cmdline="TESTNAME=$testname ACCEL=$accel __run $kernel -smp $smp $opts"
     if [ "$verbose" -a "$verbose" != 0 ]; then
-- 
2.6.4

[PATCH kvm-unit-tests v2 09/12] x86/hyperv_synic: check for support before testing

[Radim Krčmář]

It's not easy to distinguish successful unit-test from failed QEMU, so
we check for presence of the needed feature before hand.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: remove "> /dev/null" as check doesn't print the output anymore
 
 x86/unittests.cfg | 1 +
 1 file changed, 1 insertion(+)

diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 6b94ad93dcf0..25779993cc27 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -182,3 +182,4 @@ arch = x86_64
 file = hyperv_synic.flat
 smp = 2
 extra_params = -cpu kvm64,hv_synic -device hyperv-testdev
+check = echo quit | $qemu -cpu kvm64,hv_synic -device hyperv-testdev -monitor stdio
-- 
2.6.4

[PATCH kvm-unit-tests v2 10/12] run_tests: print summary

[Radim Krčmář]

Might be interesting and hopefully won't break too many scripts.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2:
 - don't print "0 unexpected failures" in run_tests' summary. [Drew]
   (This could have been done in lib/report, but I'm not sure why we want
    to always print it in the summary, so I've kept it there.)
 - use $testname
 - don't buffer the output (a serious bug in v1)
 - worse performance (reads the output of all tests)
 
 run_tests.sh            | 1 +
 scripts/mkstandalone.sh | 2 ++
 scripts/run.bash        | 5 ++++-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/run_tests.sh b/run_tests.sh
index e09d410beaa4..62c106a0b693 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -30,6 +30,7 @@ EOF
 
 __run() { ./$TEST_DIR-run "${@}"; }
 __eval_log() { eval "${@}" >> test.log; }
+__echo_last_log() { cat test.log; } # XXX: ignores the 'last' bit
 
 echo > test.log
 
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index 778383077769..c37f694398b8 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -104,7 +104,9 @@ __run()
 	\$qemu \$cmdline -smp $smp $opts
 }
 
+# log goes to stdout and nothing is remembered
 __eval_log() { eval "\${@}"; }
+__echo_last_log() { echo; }
 
 run `escape "${@}"`
 exit \$?
diff --git a/scripts/run.bash b/scripts/run.bash
index f532cb9e8b1c..d3e8d37d315d 100644
--- a/scripts/run.bash
+++ b/scripts/run.bash
@@ -46,7 +46,10 @@ function run()
     *)  echo -ne "\e[31mFAIL\e[0m"
     esac
 
-    echo " $1"
+    echo -n " $testname"
+
+    __echo_last_log | sed 'x;$s/^SUMMARY: //;ta;$s/.*//p;d;
+                             :a s/, 0 unexpected failures//;s/^/ (/;s/$/)/'
 
     return $ret
 }
-- 
2.6.4

[PATCH kvm-unit-tests v2 11/12] wrappers: consolidate skip output

[Radim Krčmář]

Ugly helpers will get us yellow "SKIP" to stdout and 77 on exit.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: new
 
 scripts/mkstandalone.sh |  6 +++---
 scripts/run.bash        | 25 ++++++++++++++++---------
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index c37f694398b8..adb11abf650c 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -66,7 +66,7 @@ cat scripts/run.bash >> $standalone
 
 if [ ! -f $kernel ]; then
 	cat <<EOF >> $standalone
-echo "skip $testname (test kernel not present)" 1>&2
+echo "\$(SKIP) $testname (test kernel not present)"
 exit 1
 EOF
 else
@@ -89,8 +89,8 @@ echo \$qemu $cmdline -smp $smp $opts
 
 cmdline="\`echo '$cmdline' | sed s%$kernel%_NO_FILE_4Uhere_%\`"
 if \$qemu \$cmdline 2>&1 | grep 'No accelerator found' >/dev/null; then
-	echo "skip $testname (QEMU doesn't support KVM)"
-	exit 1
+	echo "\$(SKIP) $testname (QEMU doesn't support KVM)"
+	exit \$EXIT_SKIP
 fi
 
 __run()
diff --git a/scripts/run.bash b/scripts/run.bash
index d3e8d37d315d..06a13b9aaf1a 100644
--- a/scripts/run.bash
+++ b/scripts/run.bash
@@ -1,3 +1,10 @@
+PASS() { echo -ne "\e[32mPASS\e[0m"; }
+SKIP() { echo -ne "\e[33mSKIP\e[0m"; }
+FAIL() { echo -ne "\e[31mFAIL\e[0m"; }
+
+EXIT_SUCCESS=0
+EXIT_SKIP=77
+
 function run()
 {
     local testname="$1"
@@ -10,22 +17,22 @@ function run()
     local accel="$8"
 
     if [ -z "$testname" ]; then
-        return
+        return $EXIT_SKIP
     fi
 
     if [ -n "$only_group" ] && ! grep -q "$only_group" <<<$groups; then
-        return
+        return $EXIT_SKIP
     fi
 
     if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
-        echo "skip $1 ($arch only)"
-        return
+        echo "`SKIP` $testname ($arch only)"
+        return $EXIT_SKIP
     fi
 
     __eval_log "$check" || {
         __eval_log 'echo "skipped $testname (check returned $?)"'
-        echo "skip $testname (failed check)"
-        return
+        echo "`SKIP` $testname (failed check)"
+        return $EXIT_SKIP
     }
 
     cmdline="TESTNAME=$testname ACCEL=$accel __run $kernel -smp $smp $opts"
@@ -41,9 +48,9 @@ function run()
     ret=$(($? >> 1))
 
     case $ret in
-    0)  echo -ne "\e[32mPASS\e[0m" ;;
-    77) echo -ne "\e[33mSKIP\e[0m" ;;
-    *)  echo -ne "\e[31mFAIL\e[0m"
+    $EXIT_SUCCESS) PASS ;;
+    $EXIT_SKIP)    SKIP ;;
+    *)             FAIL
     esac
 
     echo -n " $testname"
-- 
2.6.4

[PATCH kvm-unit-tests v2 12/12] run_tests: suppress stderr

[Radim Krčmář]

log it instead to keep the output clear.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: new (v1 did this by catching all output in a variable)

 run_tests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/run_tests.sh b/run_tests.sh
index 62c106a0b693..4c8c20a16eac 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -29,7 +29,7 @@ EOF
 }
 
 __run() { ./$TEST_DIR-run "${@}"; }
-__eval_log() { eval "${@}" >> test.log; }
+__eval_log() { eval "${@}" >> test.log 2>&1; }
 __echo_last_log() { cat test.log; } # XXX: ignores the 'last' bit
 
 echo > test.log
-- 
2.6.4

Re: [PATCH kvm-unit-tests v2 01/12] run_tests: move run() to scripts/

[Andrew Jones]

On Thu, Dec 17, 2015 at 06:53:32PM +0100, Radim Krčmář wrote:
> We'll be using it from scripts/mkstandalone later.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: new
>  
>  run_tests.sh     | 53 +----------------------------------------------------
>  scripts/run.bash | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++

Could probably just put run() in scripts/functions.bash

drew

Re: [PATCH kvm-unit-tests v2 02/12] run_tests: prepare for changes in scripts/mkstandalone

[Andrew Jones]

On Thu, Dec 17, 2015 at 06:53:33PM +0100, Radim Krčmář wrote:
> mkstandalone has a different mechanism for running tests as well as a
> different handling of output and return codes.
>  - create two shell function to capture test execution and logging
>  - return the return value of unit-test
>  - cope with empty $verbose in `run`
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: new (reused the bitshift and comment from v1)
>  
>  run_tests.sh     |  4 ++++
>  scripts/run.bash | 13 +++++++++----
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/run_tests.sh b/run_tests.sh
> index 58949e39c38c..e09d410beaa4 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -28,7 +28,11 @@ specify the appropriate qemu binary for ARCH-run.
>  EOF
>  }
>  
> +__run() { ./$TEST_DIR-run "${@}"; }
> +__eval_log() { eval "${@}" >> test.log; }
> +
>  echo > test.log
> +
>  while getopts "g:hv" opt; do
>      case $opt in
>          g)
> diff --git a/scripts/run.bash b/scripts/run.bash
> index 0c5a2569d80e..243586c6d2fc 100644
> --- a/scripts/run.bash
> +++ b/scripts/run.bash
> @@ -34,18 +34,23 @@ function run()
>          fi
>      done
>  
> -    cmdline="TESTNAME=$testname ACCEL=$accel ./$TEST_DIR-run $kernel -smp $smp $opts"
> -    if [ $verbose != 0 ]; then
> +    cmdline="TESTNAME=$testname ACCEL=$accel __run $kernel -smp $smp $opts"
> +    if [ "$verbose" -a "$verbose" != 0 ]; then

For bash bools I prefer just doing 'if [ "$verbose" = "yes" ]', allowing it
to be empty.

>          echo $cmdline
>      fi
>  
>      # extra_params in the config file may contain backticks that need to be
>      # expanded, so use eval to start qemu
> -    eval $cmdline >> test.log
> +    __eval_log "$cmdline"
> +    # The first bit of return value is too hard to use, just skip it.
> +    # Unit-tests' return value is shifted by one.
> +    ret=$(($? >> 1))

I just wrote a patch, inspired by reviewing your v1 of this series, that
tackles the ambiguous exit code problem. I'll post it now, but obviously
we'll need to rebase one or the other of our run_tests.sh series'.

>  
> -    if [ $? -le 1 ]; then
> +    if [ $ret -eq 0 ]; then
>          echo -e "\e[32mPASS\e[0m $1"
>      else
>          echo -e "\e[31mFAIL\e[0m $1"
>      fi
> +
> +    return $ret
>  }
> -- 
> 2.6.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests v2 03/12] scripts/mkstandalone: use common run function

[Andrew Jones]

On Thu, Dec 17, 2015 at 06:53:34PM +0100, Radim Krčmář wrote:
> The biggest change is dependency on bash.  An alternative would be to
> rewrite `run` in POSIX shell, but I think it's ok to presume that KVM
> unit tests will run on a system where installing bash isn't a problem.

Hmm... as hard as I worked to avoid the dependency on bash for the
standalone tests, then I'm reluctant to give up on that. I do agree
that having the dependency for the printf-%q trick helps a ton in
making the code more maintainable though.

> (We already depend on QEMU ...)

Dependency on qemu doesn't imply a dependency on bash. The idea behind
the standalone version of kvm-unit-tests tests is that you can receive
one in your email and launch it.

> 
> Apart from that, there are changes in output and exit codes.
>  - summary doesn't go to stderr

I wanted the summary on stderr so when you redirect the output of the
test to a file the output would directly diff with the corresponding
output in test.log from a system where run_tests.sh was used.

>  - PASS/FAIL is colourful
>  - FAILed scripts return 1

I'm not sure why I did exit 0 instead of exit $ret. I guess that was
just a thinko on my part. exit $ret makes more sense.

> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: new (I can fix the stderr in v3)
>  
>  scripts/mkstandalone.sh | 59 +++++++++++++++++++++----------------------------
>  1 file changed, 25 insertions(+), 34 deletions(-)
> 
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> index 3ce244aff67b..cf2182dbd936 100755
> --- a/scripts/mkstandalone.sh
> +++ b/scripts/mkstandalone.sh
> @@ -20,6 +20,13 @@ fi
>  unittests=$TEST_DIR/unittests.cfg
>  mkdir -p tests
>  
> +escape ()
> +{
> +	for arg in "${@}"; do
> +		printf "%q " "$arg"; # XXX: trailing whitespace
> +	done
> +}
> +
>  function mkstandalone()
>  {
>  	local testname="$1"
> @@ -49,33 +56,14 @@ function mkstandalone()
>  	cmdline=$(cut -d' ' -f2- <<< "$cmdline")
>  
>  	cat <<EOF > $standalone
> -#!/bin/sh
> +#!/bin/bash
>  
> -EOF
> -if [ "$arch" ]; then
> -	cat <<EOF >> $standalone
>  ARCH=\`uname -m | sed -e s/i.86/i386/ | sed -e 's/arm.*/arm/'\`
> -[ "\$ARCH" = "aarch64" ] && ARCH="arm64"
> -if [ "\$ARCH" != "$arch" ]; then
> -	echo "skip $testname ($arch only)" 1>&2
> -	exit 1
> -fi
>  
>  EOF
> -fi
> -if [ "$check" ]; then
> -	cat <<EOF >> $standalone
> -for param in $check; do
> -	path=\`echo \$param | cut -d= -f1\`
> -	value=\`echo \$param | cut -d= -f2\`
> -	if [ -f "\$path" ] && [ "\`cat \$path\`" != "\$value" ]; then
> -		echo "skip $testname (\$path not equal to \$value)" 1>&2
> -		exit 1
> -	fi
> -done
>  
> -EOF
> -fi
> +cat scripts/run.bash >> $standalone
> +
>  if [ ! -f $kernel ]; then
>  	cat <<EOF >> $standalone
>  echo "skip $testname (test kernel not present)" 1>&2
> @@ -100,9 +88,13 @@ MAX_SMP="MAX_SMP"
>  echo \$qemu $cmdline -smp $smp $opts
>  
>  cmdline="\`echo '$cmdline' | sed s%$kernel%_NO_FILE_4Uhere_%\`"
> -if \$qemu \$cmdline 2>&1 | grep 'No accelerator found'; then
> -	ret=2
> -else
> +if \$qemu \$cmdline 2>&1 | grep 'No accelerator found' >/dev/null; then
> +	echo "skip $testname (QEMU doesn't support KVM)"
> +	exit 1
> +fi
> +
> +__run()
> +{
>  	MAX_SMP=\`getconf _NPROCESSORS_CONF\`
>  	while \$qemu \$cmdline -smp \$MAX_SMP 2>&1 | grep 'exceeds max cpus' > /dev/null; do
>  		MAX_SMP=\`expr \$MAX_SMP - 1\`
> @@ -110,16 +102,15 @@ else
>  
>  	cmdline="\`echo '$cmdline' | sed s%$kernel%\$bin%\`"
>  	\$qemu \$cmdline -smp $smp $opts
> -	ret=\$?
> -fi
> -echo Return value from qemu: \$ret
> -if [ \$ret -le 1 ]; then
> -	echo PASS $testname 1>&2
> -else
> -	echo FAIL $testname 1>&2
> -fi
> +}
> +
> +__eval_log() { eval "\${@}"; }
> +
> +run `escape "${@}"`
> +ret=$?
> +
>  rm -f \$bin
> -exit 0
> +exit \$ret
>  EOF
>  fi
>  chmod +x $standalone
> -- 
> 2.6.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests v2 04/12] scripts/mkstandalone: improve exit paths

[Andrew Jones]

On Thu, Dec 17, 2015 at 06:53:35PM +0100, Radim Krčmář wrote:
> trap can be called on EXIT, which covers most exits.

Not with dash :-) If we decide to depend on bash, then

Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: new
>  
>  scripts/mkstandalone.sh | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> index cf2182dbd936..778383077769 100755
> --- a/scripts/mkstandalone.sh
> +++ b/scripts/mkstandalone.sh
> @@ -71,7 +71,7 @@ exit 1
>  EOF
>  else
>  	cat <<EOF >> $standalone
> -trap 'rm -f \$bin; exit 1' HUP INT TERM
> +trap 'rm -f \$bin' EXIT
>  bin=\`mktemp\`
>  base64 -d << 'BIN_EOF' | zcat > \$bin &&
>  EOF
> @@ -107,10 +107,7 @@ __run()
>  __eval_log() { eval "\${@}"; }
>  
>  run `escape "${@}"`
> -ret=$?
> -
> -rm -f \$bin
> -exit \$ret
> +exit \$?
>  EOF
>  fi
>  chmod +x $standalone
> -- 
> 2.6.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests v2 05/12] lib/report: allow test skipping

[Andrew Jones]

On Thu, Dec 17, 2015 at 06:53:36PM +0100, Radim Krčmář wrote:
> We can now explicitly mark a unit-test as skipped.
> If all unit-tests were skipped, the whole test is reported as skipped as
> well.  This also includes the case where no tests were run, but still
> ended with report_summary().
> 
> When the whole test is skipped, ./run_tests.sh prints yellow "SKIP"
> instead of green "PASS".
> 
> Return value of 77 is used to please Autotools.  I also renamed few
> things in reporting code and chose to refactor a logic while at it.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2:
>  - turned skip into yellow SKIP [Drew]
>  - wrapped line at 80 characters [Drew]
>  - added static to va_report
>  
>  lib/libcflat.h   |  1 +
>  lib/report.c     | 44 ++++++++++++++++++++++++++++++--------------
>  scripts/run.bash | 12 +++++++-----
>  3 files changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/libcflat.h b/lib/libcflat.h
> index 9747ccdbc9f1..070818354ee1 100644
> --- a/lib/libcflat.h
> +++ b/lib/libcflat.h
> @@ -61,6 +61,7 @@ void report_prefix_push(const char *prefix);
>  void report_prefix_pop(void);
>  void report(const char *msg_fmt, bool pass, ...);
>  void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...);
> +void report_skip(const char *msg_fmt, ...);
>  int report_summary(void);
>  
>  #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0]))
> diff --git a/lib/report.c b/lib/report.c
> index 35e664108a92..a47f2e00b529 100644
> --- a/lib/report.c
> +++ b/lib/report.c
> @@ -13,7 +13,7 @@
>  #include "libcflat.h"
>  #include "asm/spinlock.h"
>  
> -static unsigned int tests, failures, xfailures;
> +static unsigned int tests, failures, xfailures, skipped;
>  static char prefixes[256];
>  static struct spinlock lock;
>  
> @@ -43,25 +43,28 @@ void report_prefix_pop(void)
>  	spin_unlock(&lock);
>  }
>  
> -void va_report_xfail(const char *msg_fmt, bool xfail, bool cond, va_list va)
> +static void va_report(const char *msg_fmt, bool pass, bool xfail, bool skip,
> +		va_list va)

Making this static disallows unit test writers to create their own
variable arg report() wrapper functions. Perhaps to determine whether
or not a skip is in order, e.g.

 xyz_report(msg, pass, ...)
 {
    va_list va;
    va_start(va, pass);
    if (xyz)
       va_report(msg, pass, false, false, va);
    else
       va_report(msg, false, false, true, va);
    va_end(va);
 }

>  {
> -	char *pass = xfail ? "XPASS" : "PASS";
> -	char *fail = xfail ? "XFAIL" : "FAIL";
>  	char buf[2000];
> +	char *prefix = skip ? "SKIP"
> +	                    : xfail ? (pass ? "XPASS" : "XFAIL")
> +	                            : (pass ? "PASS"  : "FAIL");
>  
>  	spin_lock(&lock);
>  
>  	tests++;
> -	printf("%s: ", cond ? pass : fail);
> +	printf("%s: ", prefix);
>  	puts(prefixes);
>  	vsnprintf(buf, sizeof(buf), msg_fmt, va);
>  	puts(buf);
>  	puts("\n");
> -	if (xfail && cond)
> -		failures++;
> -	else if (xfail)
> +
> +	if (skip)
> +		skipped++;
> +	else if (xfail && !pass)
>  		xfailures++;
> -	else if (!cond)
> +	else if (xfail || !pass)
>  		failures++;
>  
>  	spin_unlock(&lock);
> @@ -71,7 +74,7 @@ void report(const char *msg_fmt, bool pass, ...)
>  {
>  	va_list va;
>  	va_start(va, pass);
> -	va_report_xfail(msg_fmt, false, pass, va);
> +	va_report(msg_fmt, pass, false, false, va);
>  	va_end(va);
>  }
>  
> @@ -79,7 +82,15 @@ void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...)
>  {
>  	va_list va;
>  	va_start(va, pass);
> -	va_report_xfail(msg_fmt, xfail, pass, va);
> +	va_report(msg_fmt, pass, xfail, false, va);
> +	va_end(va);
> +}
> +
> +void report_skip(const char *msg_fmt, ...)
> +{
> +	va_list va;
> +	va_start(va, msg_fmt);
> +	va_report(msg_fmt, false, false, true, va);
>  	va_end(va);
>  }
>  
> @@ -89,9 +100,14 @@ int report_summary(void)
>  
>  	printf("\nSUMMARY: %d tests, %d unexpected failures", tests, failures);
>  	if (xfailures)
> -		printf(", %d expected failures\n", xfailures);
> -	else
> -		printf("\n");
> +		printf(", %d expected failures", xfailures);
> +	if (skipped)
> +		printf(", %d skipped", skipped);
> +	printf("\n");
> +
> +	if (tests == skipped)
> +		return 77; /* blame AUTOTOOLS */
> +
>  	return failures > 0 ? 1 : 0;
>  
>  	spin_unlock(&lock);
> diff --git a/scripts/run.bash b/scripts/run.bash
> index 243586c6d2fc..b92611c29fbb 100644
> --- a/scripts/run.bash
> +++ b/scripts/run.bash
> @@ -46,11 +46,13 @@ function run()
>      # Unit-tests' return value is shifted by one.
>      ret=$(($? >> 1))
>  
> -    if [ $ret -eq 0 ]; then
> -        echo -e "\e[32mPASS\e[0m $1"
> -    else
> -        echo -e "\e[31mFAIL\e[0m $1"
> -    fi
> +    case $ret in
> +    0)  echo -ne "\e[32mPASS\e[0m" ;;
> +    77) echo -ne "\e[33mSKIP\e[0m" ;;
> +    *)  echo -ne "\e[31mFAIL\e[0m"
> +    esac
> +
> +    echo " $1"

While touching this, please change this to $testname, and anything
else in run() that still uses $N instead of its local var name.

>  
>      return $ret
>  }
> -- 
> 2.6.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests v2 05/12] lib/report: allow test skipping

[Andrew Jones]

On Thu, Dec 17, 2015 at 01:30:23PM -0600, Andrew Jones wrote:
> On Thu, Dec 17, 2015 at 06:53:36PM +0100, Radim Krčmář wrote:
> > We can now explicitly mark a unit-test as skipped.
> > If all unit-tests were skipped, the whole test is reported as skipped as
> > well.  This also includes the case where no tests were run, but still
> > ended with report_summary().
> > 
> > When the whole test is skipped, ./run_tests.sh prints yellow "SKIP"
> > instead of green "PASS".
> > 
> > Return value of 77 is used to please Autotools.  I also renamed few
> > things in reporting code and chose to refactor a logic while at it.
> > 
> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> > ---
> >  v2:
> >  - turned skip into yellow SKIP [Drew]
> >  - wrapped line at 80 characters [Drew]
> >  - added static to va_report
> >  
> >  lib/libcflat.h   |  1 +
> >  lib/report.c     | 44 ++++++++++++++++++++++++++++++--------------
> >  scripts/run.bash | 12 +++++++-----
> >  3 files changed, 38 insertions(+), 19 deletions(-)
> > 
> > diff --git a/lib/libcflat.h b/lib/libcflat.h
> > index 9747ccdbc9f1..070818354ee1 100644
> > --- a/lib/libcflat.h
> > +++ b/lib/libcflat.h
> > @@ -61,6 +61,7 @@ void report_prefix_push(const char *prefix);
> >  void report_prefix_pop(void);
> >  void report(const char *msg_fmt, bool pass, ...);
> >  void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...);
> > +void report_skip(const char *msg_fmt, ...);
> >  int report_summary(void);
> >  
> >  #define ARRAY_SIZE(_a) (sizeof(_a)/sizeof((_a)[0]))
> > diff --git a/lib/report.c b/lib/report.c
> > index 35e664108a92..a47f2e00b529 100644
> > --- a/lib/report.c
> > +++ b/lib/report.c
> > @@ -13,7 +13,7 @@
> >  #include "libcflat.h"
> >  #include "asm/spinlock.h"
> >  
> > -static unsigned int tests, failures, xfailures;
> > +static unsigned int tests, failures, xfailures, skipped;
> >  static char prefixes[256];
> >  static struct spinlock lock;
> >  
> > @@ -43,25 +43,28 @@ void report_prefix_pop(void)
> >  	spin_unlock(&lock);
> >  }
> >  
> > -void va_report_xfail(const char *msg_fmt, bool xfail, bool cond, va_list va)
> > +static void va_report(const char *msg_fmt, bool pass, bool xfail, bool skip,
> > +		va_list va)
> 
> Making this static disallows unit test writers to create their own
> variable arg report() wrapper functions. Perhaps to determine whether
> or not a skip is in order, e.g.
> 
>  xyz_report(msg, pass, ...)
>  {
>     va_list va;
>     va_start(va, pass);
>     if (xyz)
>        va_report(msg, pass, false, false, va);
>     else
>        va_report(msg, false, false, true, va);
>     va_end(va);
>  }

Hmm, while I still think we should avoid using static, to allow new wrappers,
the wrapper I wrote here as an example wouldn't be necessary if report_skip's
inputs were instead 

void report_skip(const char *msg_fmt, bool pass, bool skip, ...)

Why not do that?

> 
> >  {
> > -	char *pass = xfail ? "XPASS" : "PASS";
> > -	char *fail = xfail ? "XFAIL" : "FAIL";
> >  	char buf[2000];
> > +	char *prefix = skip ? "SKIP"
> > +	                    : xfail ? (pass ? "XPASS" : "XFAIL")
> > +	                            : (pass ? "PASS"  : "FAIL");
> >  
> >  	spin_lock(&lock);
> >  
> >  	tests++;
> > -	printf("%s: ", cond ? pass : fail);
> > +	printf("%s: ", prefix);
> >  	puts(prefixes);
> >  	vsnprintf(buf, sizeof(buf), msg_fmt, va);
> >  	puts(buf);
> >  	puts("\n");
> > -	if (xfail && cond)
> > -		failures++;
> > -	else if (xfail)
> > +
> > +	if (skip)
> > +		skipped++;
> > +	else if (xfail && !pass)
> >  		xfailures++;
> > -	else if (!cond)
> > +	else if (xfail || !pass)
> >  		failures++;
> >  
> >  	spin_unlock(&lock);
> > @@ -71,7 +74,7 @@ void report(const char *msg_fmt, bool pass, ...)
> >  {
> >  	va_list va;
> >  	va_start(va, pass);
> > -	va_report_xfail(msg_fmt, false, pass, va);
> > +	va_report(msg_fmt, pass, false, false, va);
> >  	va_end(va);
> >  }
> >  
> > @@ -79,7 +82,15 @@ void report_xfail(const char *msg_fmt, bool xfail, bool pass, ...)
> >  {
> >  	va_list va;
> >  	va_start(va, pass);
> > -	va_report_xfail(msg_fmt, xfail, pass, va);
> > +	va_report(msg_fmt, pass, xfail, false, va);
> > +	va_end(va);
> > +}
> > +
> > +void report_skip(const char *msg_fmt, ...)
> > +{
> > +	va_list va;
> > +	va_start(va, msg_fmt);
> > +	va_report(msg_fmt, false, false, true, va);
> >  	va_end(va);
> >  }
> >  
> > @@ -89,9 +100,14 @@ int report_summary(void)
> >  
> >  	printf("\nSUMMARY: %d tests, %d unexpected failures", tests, failures);
> >  	if (xfailures)
> > -		printf(", %d expected failures\n", xfailures);
> > -	else
> > -		printf("\n");
> > +		printf(", %d expected failures", xfailures);
> > +	if (skipped)
> > +		printf(", %d skipped", skipped);
> > +	printf("\n");
> > +
> > +	if (tests == skipped)
> > +		return 77; /* blame AUTOTOOLS */
> > +
> >  	return failures > 0 ? 1 : 0;
> >  
> >  	spin_unlock(&lock);
> > diff --git a/scripts/run.bash b/scripts/run.bash
> > index 243586c6d2fc..b92611c29fbb 100644
> > --- a/scripts/run.bash
> > +++ b/scripts/run.bash
> > @@ -46,11 +46,13 @@ function run()
> >      # Unit-tests' return value is shifted by one.
> >      ret=$(($? >> 1))
> >  
> > -    if [ $ret -eq 0 ]; then
> > -        echo -e "\e[32mPASS\e[0m $1"
> > -    else
> > -        echo -e "\e[31mFAIL\e[0m $1"
> > -    fi
> > +    case $ret in
> > +    0)  echo -ne "\e[32mPASS\e[0m" ;;
> > +    77) echo -ne "\e[33mSKIP\e[0m" ;;
> > +    *)  echo -ne "\e[31mFAIL\e[0m"
> > +    esac
> > +
> > +    echo " $1"
> 
> While touching this, please change this to $testname, and anything
> else in run() that still uses $N instead of its local var name.
> 
> >  
> >      return $ret
> >  }
> > -- 
> > 2.6.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests v2 09/12] x86/hyperv_synic: check for support before testing

[Andrew Jones]

On Thu, Dec 17, 2015 at 06:53:40PM +0100, Radim Krčmář wrote:
> It's not easy to distinguish successful unit-test from failed QEMU, so
> we check for presence of the needed feature before hand.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: remove "> /dev/null" as check doesn't print the output anymore
>  
>  x86/unittests.cfg | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> index 6b94ad93dcf0..25779993cc27 100644
> --- a/x86/unittests.cfg
> +++ b/x86/unittests.cfg
> @@ -182,3 +182,4 @@ arch = x86_64
>  file = hyperv_synic.flat
>  smp = 2
>  extra_params = -cpu kvm64,hv_synic -device hyperv-testdev
> +check = echo quit | $qemu -cpu kvm64,hv_synic -device hyperv-testdev -monitor stdio

Let's make sure $QEMU==$qemu in contexts where unittests.cfg is used, and
then document (in the unittests.cfg header) that $QEMU may be used in the
check lines.

> -- 
> 2.6.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests v2 10/12] run_tests: print summary

[Andrew Jones]

On Thu, Dec 17, 2015 at 06:53:41PM +0100, Radim Krčmář wrote:
> Might be interesting and hopefully won't break too many scripts.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2:
>  - don't print "0 unexpected failures" in run_tests' summary. [Drew]
>    (This could have been done in lib/report, but I'm not sure why we want
>     to always print it in the summary, so I've kept it there.)
>  - use $testname
>  - don't buffer the output (a serious bug in v1)
>  - worse performance (reads the output of all tests)
>  
>  run_tests.sh            | 1 +
>  scripts/mkstandalone.sh | 2 ++
>  scripts/run.bash        | 5 ++++-
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/run_tests.sh b/run_tests.sh
> index e09d410beaa4..62c106a0b693 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -30,6 +30,7 @@ EOF
>  
>  __run() { ./$TEST_DIR-run "${@}"; }
>  __eval_log() { eval "${@}" >> test.log; }
> +__echo_last_log() { cat test.log; } # XXX: ignores the 'last' bit
>  
>  echo > test.log
>  
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> index 778383077769..c37f694398b8 100755
> --- a/scripts/mkstandalone.sh
> +++ b/scripts/mkstandalone.sh
> @@ -104,7 +104,9 @@ __run()
>  	\$qemu \$cmdline -smp $smp $opts
>  }
>  
> +# log goes to stdout and nothing is remembered
>  __eval_log() { eval "\${@}"; }
> +__echo_last_log() { echo; }
>  
>  run `escape "${@}"`
>  exit \$?
> diff --git a/scripts/run.bash b/scripts/run.bash
> index f532cb9e8b1c..d3e8d37d315d 100644
> --- a/scripts/run.bash
> +++ b/scripts/run.bash
> @@ -46,7 +46,10 @@ function run()
>      *)  echo -ne "\e[31mFAIL\e[0m"
>      esac
>  
> -    echo " $1"
> +    echo -n " $testname"
> +
> +    __echo_last_log | sed 'x;$s/^SUMMARY: //;ta;$s/.*//p;d;
> +                             :a s/, 0 unexpected failures//;s/^/ (/;s/$/)/'

This sed is way beyond my skillz. I would have just done the following
at the expense of an extra pipe :-)

tac test.log | grep -m1 '^SUMMARY:' | sed 's/.*: \(.*\),.*/\1/'

>  
>      return $ret
>  }
> -- 
> 2.6.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests v2 11/12] wrappers: consolidate skip output

[Andrew Jones]

On Thu, Dec 17, 2015 at 06:53:42PM +0100, Radim Krčmář wrote:
> Ugly helpers will get us yellow "SKIP" to stdout and 77 on exit.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: new
>  
>  scripts/mkstandalone.sh |  6 +++---
>  scripts/run.bash        | 25 ++++++++++++++++---------
>  2 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> index c37f694398b8..adb11abf650c 100755
> --- a/scripts/mkstandalone.sh
> +++ b/scripts/mkstandalone.sh
> @@ -66,7 +66,7 @@ cat scripts/run.bash >> $standalone
>  
>  if [ ! -f $kernel ]; then
>  	cat <<EOF >> $standalone
> -echo "skip $testname (test kernel not present)" 1>&2
> +echo "\$(SKIP) $testname (test kernel not present)"
>  exit 1
>  EOF
>  else
> @@ -89,8 +89,8 @@ echo \$qemu $cmdline -smp $smp $opts
>  
>  cmdline="\`echo '$cmdline' | sed s%$kernel%_NO_FILE_4Uhere_%\`"
>  if \$qemu \$cmdline 2>&1 | grep 'No accelerator found' >/dev/null; then
> -	echo "skip $testname (QEMU doesn't support KVM)"
> -	exit 1
> +	echo "\$(SKIP) $testname (QEMU doesn't support KVM)"
> +	exit \$EXIT_SKIP
>  fi
>  
>  __run()
> diff --git a/scripts/run.bash b/scripts/run.bash
> index d3e8d37d315d..06a13b9aaf1a 100644
> --- a/scripts/run.bash
> +++ b/scripts/run.bash
> @@ -1,3 +1,10 @@
> +PASS() { echo -ne "\e[32mPASS\e[0m"; }
> +SKIP() { echo -ne "\e[33mSKIP\e[0m"; }
> +FAIL() { echo -ne "\e[31mFAIL\e[0m"; }

I definitely put these in functions.bash, but then I'd put run() in
functions.bash too, as I said earlier.

> +
> +EXIT_SUCCESS=0
> +EXIT_SKIP=77
> +
>  function run()
>  {
>      local testname="$1"
> @@ -10,22 +17,22 @@ function run()
>      local accel="$8"
>  
>      if [ -z "$testname" ]; then
> -        return
> +        return $EXIT_SKIP
>      fi
>  
>      if [ -n "$only_group" ] && ! grep -q "$only_group" <<<$groups; then
> -        return
> +        return $EXIT_SKIP
>      fi
>  
>      if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
> -        echo "skip $1 ($arch only)"
> -        return
> +        echo "`SKIP` $testname ($arch only)"
> +        return $EXIT_SKIP
>      fi
>  
>      __eval_log "$check" || {
>          __eval_log 'echo "skipped $testname (check returned $?)"'
> -        echo "skip $testname (failed check)"
> -        return
> +        echo "`SKIP` $testname (failed check)"
> +        return $EXIT_SKIP
>      }
>  
>      cmdline="TESTNAME=$testname ACCEL=$accel __run $kernel -smp $smp $opts"
> @@ -41,9 +48,9 @@ function run()
>      ret=$(($? >> 1))
>  
>      case $ret in
> -    0)  echo -ne "\e[32mPASS\e[0m" ;;
> -    77) echo -ne "\e[33mSKIP\e[0m" ;;
> -    *)  echo -ne "\e[31mFAIL\e[0m"
> +    $EXIT_SUCCESS) PASS ;;
> +    $EXIT_SKIP)    SKIP ;;
> +    *)             FAIL
>      esac
>  
>      echo -n " $testname"
> -- 
> 2.6.4

Otherwise
Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests v2 12/12] run_tests: suppress stderr

[Andrew Jones]

On Thu, Dec 17, 2015 at 06:53:43PM +0100, Radim Krčmář wrote:
> log it instead to keep the output clear.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: new (v1 did this by catching all output in a variable)
> 
>  run_tests.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/run_tests.sh b/run_tests.sh
> index 62c106a0b693..4c8c20a16eac 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -29,7 +29,7 @@ EOF
>  }
>  
>  __run() { ./$TEST_DIR-run "${@}"; }
> -__eval_log() { eval "${@}" >> test.log; }
> +__eval_log() { eval "${@}" >> test.log 2>&1; }
>  __echo_last_log() { cat test.log; } # XXX: ignores the 'last' bit

This will greatly conflict with my exit code untangling patch. I'll send
that now so you can see it.

>  
>  echo > test.log
> -- 
> 2.6.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests v2 00/12] Improve the output of test runners

[Andrew Jones]

On Thu, Dec 17, 2015 at 06:53:31PM +0100, Radim Krčmář wrote:
> v1: http://www.spinics.net/lists/kvm/msg125202.html
> 
> Drew brought up the existence of scripts/mkstandalone.sh, which
> significantly expanded v2 (and my set of curses) ...
> I didn't want to do the same twice, so first part of this series,
> [1-4/12], reuses run() from run_tests.sh and does some non-conservative
> changes to scripts/mkstandalone.sh.  scripts/mkstandalone.sh is lacking
> behind run_tests.sh, but should be good enough to fulfill its purpose.
> 
> The output of run_tests.sh has also changed a bit and now looks like
> this (you'll again need to imagine colours):
> 
> > PASS apic (14 tests)
> > PASS ioapic (19 tests)
> > PASS smptest (1 tests)
> > PASS smptest3 (1 tests)
> > PASS vmexit_cpuid
> > PASS vmexit_vmcall

Why do some tests, which have only 1 test, say (1 tests), but other
tests don't say anything?

> > PASS vmexit_mov_from_cr8
> > PASS vmexit_mov_to_cr8
> > PASS vmexit_inl_pmtimer
> > PASS vmexit_ipi
> > PASS vmexit_ipi_halt
> > PASS vmexit_ple_round_robin
> > PASS access
> > SKIP smap (0 tests)
> > SKIP pku (0 tests)
> > PASS emulator (132 tests, 1 skipped)
> > PASS eventinj (13 tests)
> > PASS hypercall (2 tests)
> > PASS idt_test (4 tests)
> > PASS msr (13 tests)
> > PASS pmu (67 tests, 1 expected failures)
> > PASS port80
> > PASS realmode
> > PASS s3
> > PASS sieve
> > PASS tsc (3 tests)
> > PASS tsc_adjust (5 tests)
> > PASS xsave (17 tests)
> > PASS rmap_chain
> > SKIP svm (0 tests)
> > SKIP svm-disabled (0 tests)
> > SKIP taskswitch (i386 only)
> > SKIP taskswitch2 (i386 only)
> > PASS kvmclock_test
> > PASS pcid (3 tests)
> > SKIP vmx (0 tests)
> > PASS debug (7 tests)
> > SKIP hyperv_synic (failed check)

Some nice improvements with this series. I'm not sure I like depending
on bash in standalone tests, but then that could just be cause I worked
pretty hard at avoiding the dependency, and thus I'll have to cry at the
loss of it...

Please review the series I'll send in about 2 minutes, so we can discuss
how to integrate them.

Thanks,
drew

> 
> 
> Radim Krčmář (12):
>   run_tests: move run() to scripts/
>   run_tests: prepare for changes in scripts/mkstandalone
>   scripts/mkstandalone: use common run function
>   scripts/mkstandalone: improve exit paths
>   lib/report: allow test skipping
>   x86/*: report skipped tests
>   x86/pmu: expect failure with nmi_watchdog
>   scripts/run: generalize check
>   x86/hyperv_synic: check for support before testing
>   run_tests: print summary
>   wrappers: consolidate skip output
>   run_tests: suppress stderr
> 
>  lib/libcflat.h          |  1 +
>  lib/report.c            | 44 +++++++++++++++++++++++-----------
>  run_tests.sh            | 58 +++++---------------------------------------
>  scripts/mkstandalone.sh | 64 +++++++++++++++++++++----------------------------
>  scripts/run.bash        | 62 +++++++++++++++++++++++++++++++++++++++++++++++
>  x86/apic.c              |  7 +++---
>  x86/emulator.c          |  2 +-
>  x86/hyperv_synic.c      |  2 +-
>  x86/pku.c               |  2 +-
>  x86/pmu.c               | 11 +++++++--
>  x86/smap.c              |  2 +-
>  x86/svm.c               |  2 +-
>  x86/tsc.c               |  2 +-
>  x86/unittests.cfg       |  4 ++--
>  14 files changed, 146 insertions(+), 117 deletions(-)
>  create mode 100644 scripts/run.bash
> 
> -- 
> 2.6.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests v2 10/12] run_tests: print summary

[Andrew Jones]

On Thu, Dec 17, 2015 at 06:53:41PM +0100, Radim Krčmář wrote:
> Might be interesting and hopefully won't break too many scripts.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2:
>  - don't print "0 unexpected failures" in run_tests' summary. [Drew]
>    (This could have been done in lib/report, but I'm not sure why we want
>     to always print it in the summary, so I've kept it there.)

I vote we kill it in lib/report. It's usually just noise.

drew

Re: [PATCH kvm-unit-tests v2 01/12] run_tests: move run() to scripts/

[Radim Krčmář]

2015-12-17 12:45-0600, Andrew Jones:
> On Thu, Dec 17, 2015 at 06:53:32PM +0100, Radim Krčmář wrote:
>> We'll be using it from scripts/mkstandalone later.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>>  v2: new
>>  
>>  run_tests.sh     | 53 +----------------------------------------------------
>>  scripts/run.bash | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> Could probably just put run() in scripts/functions.bash

Definitely.  The drawback is that for_each_unittest() and any future
helper would be pasted in each unit test, which would complicate
conversion back to shell and uselessly bloat tests.  Even though I like
programming in shell far more than in C, I would not do it if we decide
to POSIX our standalone tests.  (Bloating is not an issue for me.)

Re: [PATCH kvm-unit-tests v2 02/12] run_tests: prepare for changes in scripts/mkstandalone

[Radim Krčmář]

2015-12-17 12:53-0600, Andrew Jones:
> On Thu, Dec 17, 2015 at 06:53:33PM +0100, Radim Krčmář wrote:
>> mkstandalone has a different mechanism for running tests as well as a
>> different handling of output and return codes.
>>  - create two shell function to capture test execution and logging
>>  - return the return value of unit-test
>>  - cope with empty $verbose in `run`
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>>  v2: new (reused the bitshift and comment from v1)
>>  
>> diff --git a/run_tests.sh b/run_tests.sh
>> -    cmdline="TESTNAME=$testname ACCEL=$accel ./$TEST_DIR-run $kernel -smp $smp $opts"
>> -    if [ $verbose != 0 ]; then
>> +    cmdline="TESTNAME=$testname ACCEL=$accel __run $kernel -smp $smp $opts"
>> +    if [ "$verbose" -a "$verbose" != 0 ]; then
> 
> For bash bools I prefer just doing 'if [ "$verbose" = "yes" ]', allowing it
> to be empty.

Yeah, I guess it's a bit better.
(I prefer just [ "$verbose" ].)

>>      # extra_params in the config file may contain backticks that need to be
>>      # expanded, so use eval to start qemu
>> -    eval $cmdline >> test.log
>> +    __eval_log "$cmdline"
>> +    # The first bit of return value is too hard to use, just skip it.
>> +    # Unit-tests' return value is shifted by one.
>> +    ret=$(($? >> 1))
> 
> I just wrote a patch, inspired by reviewing your v1 of this series, that
> tackles the ambiguous exit code problem. I'll post it now, but obviously
> we'll need to rebase one or the other of our run_tests.sh series'.

Nice, I'll review it later today (I'll travel for most of the afternoon).

Re: [PATCH kvm-unit-tests v2 03/12] scripts/mkstandalone: use common run function

[Radim Krčmář]

2015-12-17 13:09-0600, Andrew Jones:
> On Thu, Dec 17, 2015 at 06:53:34PM +0100, Radim Krčmář wrote:
>> The biggest change is dependency on bash.  An alternative would be to
>> rewrite `run` in POSIX shell, but I think it's ok to presume that KVM
>> unit tests will run on a system where installing bash isn't a problem.
> 
> Hmm... as hard as I worked to avoid the dependency on bash for the
> standalone tests, then I'm reluctant to give up on that. I do agree
> that having the dependency for the printf-%q trick helps a ton in
> making the code more maintainable though.

And parts of run() would need to be rewritten as well ... I am not sure
it's worth to invest the time.

>> (We already depend on QEMU ...)
> 
> Dependency on qemu doesn't imply a dependency on bash. The idea behind
> the standalone version of kvm-unit-tests tests is that you can receive
> one in your email and launch it.

No, but its quite likely that the host has bash when something as big as
QEMU is already there, or at least that it shouldn't be a huge problem
to install it.

>> Apart from that, there are changes in output and exit codes.
>>  - summary doesn't go to stderr
> 
> I wanted the summary on stderr so when you redirect the output of the
> test to a file the output would directly diff with the corresponding
> output in test.log from a system where run_tests.sh was used.

Ok, stderr from qemu needs to be redirected too then.
I will add an extra param to set file for __*log helpers or helpers that
redirect to stderr.

Re: [PATCH kvm-unit-tests v2 05/12] lib/report: allow test skipping

[Radim Krčmář]

2015-12-17 13:37-0600, Andrew Jones:
> On Thu, Dec 17, 2015 at 01:30:23PM -0600, Andrew Jones wrote:
>> On Thu, Dec 17, 2015 at 06:53:36PM +0100, Radim Krčmář wrote:
>> > We can now explicitly mark a unit-test as skipped.
>> > If all unit-tests were skipped, the whole test is reported as skipped as
>> > well.  This also includes the case where no tests were run, but still
>> > ended with report_summary().
>> > 
>> > When the whole test is skipped, ./run_tests.sh prints yellow "SKIP"
>> > instead of green "PASS".
>> > 
>> > Return value of 77 is used to please Autotools.  I also renamed few
>> > things in reporting code and chose to refactor a logic while at it.
>> > 
>> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> > ---
>> > diff --git a/lib/report.c b/lib/report.c
>> > @@ -43,25 +43,28 @@ void report_prefix_pop(void)
>> > -void va_report_xfail(const char *msg_fmt, bool xfail, bool cond, va_list va)
>> > +static void va_report(const char *msg_fmt, bool pass, bool xfail, bool skip,
>> > +		va_list va)
>> 
>> Making this static disallows unit test writers to create their own
>> variable arg report() wrapper functions. Perhaps to determine whether
>> or not a skip is in order, e.g.
>> 
>>  xyz_report(msg, pass, ...)
>>  {
>>     va_list va;
>>     va_start(va, pass);
>>     if (xyz)
>>        va_report(msg, pass, false, false, va);
>>     else
>>        va_report(msg, false, false, true, va);
>>     va_end(va);
>>  }
> 
> Hmm, while I still think we should avoid using static, to allow new wrappers,
> the wrapper I wrote here as an example wouldn't be necessary if report_skip's
> inputs were instead 

That breaks encapsulation -- if we ever want to change va_report(),
we've just made our lives harder.

> void report_skip(const char *msg_fmt, bool pass, bool skip, ...)
> 
> Why not do that?

Yeah, some cases want to unconditionally skip, so we'd want to have
both.  I'll think of naming during lunch :)

Re: [PATCH kvm-unit-tests v2 09/12] x86/hyperv_synic: check for support before testing

[Radim Krčmář]

2015-12-17 13:42-0600, Andrew Jones:
> On Thu, Dec 17, 2015 at 06:53:40PM +0100, Radim Krčmář wrote:
>> It's not easy to distinguish successful unit-test from failed QEMU, so
>> we check for presence of the needed feature before hand.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>>  v2: remove "> /dev/null" as check doesn't print the output anymore
>> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
>> @@ -182,3 +182,4 @@ arch = x86_64
>>  file = hyperv_synic.flat
>>  smp = 2
>>  extra_params = -cpu kvm64,hv_synic -device hyperv-testdev
>> +check = echo quit | $qemu -cpu kvm64,hv_synic -device hyperv-testdev -monitor stdio
> 
> Let's make sure $QEMU==$qemu in contexts where unittests.cfg is used, and
> then document (in the unittests.cfg header) that $QEMU may be used in the
> check lines.

I'd just get rid of $qemu, we don't gain anything by having them both.
Documenting it is a good idea.

Re: [PATCH kvm-unit-tests v2 10/12] run_tests: print summary

[Radim Krčmář]

2015-12-17 13:55-0600, Andrew Jones:
> On Thu, Dec 17, 2015 at 06:53:41PM +0100, Radim Krčmář wrote:
>> Might be interesting and hopefully won't break too many scripts.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>>  v2:
>>  - don't print "0 unexpected failures" in run_tests' summary. [Drew]
>>    (This could have been done in lib/report, but I'm not sure why we want
>>     to always print it in the summary, so I've kept it there.)
>>  - use $testname
>>  - don't buffer the output (a serious bug in v1)
>>  - worse performance (reads the output of all tests)
>>  
>> diff --git a/run_tests.sh b/run_tests.sh
>> +    __echo_last_log | sed 'x;$s/^SUMMARY: //;ta;$s/.*//p;d;
>> +                             :a s/, 0 unexpected failures//;s/^/ (/;s/$/)/'
> 
> This sed is way beyond my skillz. I would have just done the following
> at the expense of an extra pipe :-)
> 
> tac test.log | grep -m1 '^SUMMARY:' | sed 's/.*: \(.*\),.*/\1/'

This won't print a newline in case there is no summary, but yeah, I will
limit myself to the well known 's' command.

(tail won't buffer the whole file and `head` instead of -m is POSIX.)

Re: [PATCH kvm-unit-tests v2 10/12] run_tests: print summary

[Radim Krčmář]

2015-12-17 14:06-0600, Andrew Jones:
> On Thu, Dec 17, 2015 at 06:53:41PM +0100, Radim Krčmář wrote:
>> Might be interesting and hopefully won't break too many scripts.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>>  v2:
>>  - don't print "0 unexpected failures" in run_tests' summary. [Drew]
>>    (This could have been done in lib/report, but I'm not sure why we want
>>     to always print it in the summary, so I've kept it there.)
> 
> I vote we kill it in lib/report. It's usually just noise.

I support this.

Re: [PATCH kvm-unit-tests v2 00/12] Improve the output of test runners

[Radim Krčmář]

2015-12-17 14:04-0600, Andrew Jones:
> On Thu, Dec 17, 2015 at 06:53:31PM +0100, Radim Krčmář wrote:
>> v1: http://www.spinics.net/lists/kvm/msg125202.html
>> 
>> Drew brought up the existence of scripts/mkstandalone.sh, which
>> significantly expanded v2 (and my set of curses) ...
>> I didn't want to do the same twice, so first part of this series,
>> [1-4/12], reuses run() from run_tests.sh and does some non-conservative
>> changes to scripts/mkstandalone.sh.  scripts/mkstandalone.sh is lacking
>> behind run_tests.sh, but should be good enough to fulfill its purpose.
>> 
>> The output of run_tests.sh has also changed a bit and now looks like
>> this (you'll again need to imagine colours):
>> 
>> > PASS apic (14 tests)
>> > PASS ioapic (19 tests)
>> > PASS smptest (1 tests)
>> > PASS smptest3 (1 tests)
>> > PASS vmexit_cpuid
>> > PASS vmexit_vmcall
> 
> Why do some tests, which have only 1 test, say (1 tests), but other
> tests don't say anything?

Some tests don't use lib/report, hence don't print "^SUMMARY:", so we
don't really want to know what happens there.

> Some nice improvements with this series. I'm not sure I like depending
> on bash in standalone tests, but then that could just be cause I worked
> pretty hard at avoiding the dependency, and thus I'll have to cry at the
> loss of it...

Knowing the percentage of KVM+QEMU installations without bash would
help.  I expect it to be very close to zero, which makes compassion hard
to find ... sorry.

> Please review the series I'll send in about 2 minutes, so we can discuss
> how to integrate them.

Will do, thanks.  (Please excuse the delay.)

Re: [PATCH kvm-unit-tests v2 01/12] run_tests: move run() to scripts/

[Andrew Jones]

On Fri, Dec 18, 2015 at 11:42:40AM +0100, Radim Krčmář wrote:
> 2015-12-17 12:45-0600, Andrew Jones:
> > On Thu, Dec 17, 2015 at 06:53:32PM +0100, Radim Krčmář wrote:
> >> We'll be using it from scripts/mkstandalone later.
> >> 
> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> >> ---
> >>  v2: new
> >>  
> >>  run_tests.sh     | 53 +----------------------------------------------------
> >>  scripts/run.bash | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > 
> > Could probably just put run() in scripts/functions.bash
> 
> Definitely.  The drawback is that for_each_unittest() and any future
> helper would be pasted in each unit test, which would complicate
> conversion back to shell and uselessly bloat tests.  Even though I like

Oh right, nevermind then. If we're going to 'cat run.bash >>' then it
needs to be separate. But do we really want to 'cat run.bash'? I guess I
need to look at your mkstandalone changes again.

> programming in shell far more than in C, I would not do it if we decide
> to POSIX our standalone tests.  (Bloating is not an issue for me.)
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests v2 05/12] lib/report: allow test skipping

[Andrew Jones]

On Fri, Dec 18, 2015 at 12:18:19PM +0100, Radim Krčmář wrote:
> 2015-12-17 13:37-0600, Andrew Jones:
> > On Thu, Dec 17, 2015 at 01:30:23PM -0600, Andrew Jones wrote:
> >> On Thu, Dec 17, 2015 at 06:53:36PM +0100, Radim Krčmář wrote:
> >> > We can now explicitly mark a unit-test as skipped.
> >> > If all unit-tests were skipped, the whole test is reported as skipped as
> >> > well.  This also includes the case where no tests were run, but still
> >> > ended with report_summary().
> >> > 
> >> > When the whole test is skipped, ./run_tests.sh prints yellow "SKIP"
> >> > instead of green "PASS".
> >> > 
> >> > Return value of 77 is used to please Autotools.  I also renamed few
> >> > things in reporting code and chose to refactor a logic while at it.
> >> > 
> >> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> >> > ---
> >> > diff --git a/lib/report.c b/lib/report.c
> >> > @@ -43,25 +43,28 @@ void report_prefix_pop(void)
> >> > -void va_report_xfail(const char *msg_fmt, bool xfail, bool cond, va_list va)
> >> > +static void va_report(const char *msg_fmt, bool pass, bool xfail, bool skip,
> >> > +		va_list va)
> >> 
> >> Making this static disallows unit test writers to create their own
> >> variable arg report() wrapper functions. Perhaps to determine whether
> >> or not a skip is in order, e.g.
> >> 
> >>  xyz_report(msg, pass, ...)
> >>  {
> >>     va_list va;
> >>     va_start(va, pass);
> >>     if (xyz)
> >>        va_report(msg, pass, false, false, va);
> >>     else
> >>        va_report(msg, false, false, true, va);
> >>     va_end(va);
> >>  }
> > 
> > Hmm, while I still think we should avoid using static, to allow new wrappers,
> > the wrapper I wrote here as an example wouldn't be necessary if report_skip's
> > inputs were instead 
> 
> That breaks encapsulation -- if we ever want to change va_report(),
> we've just made our lives harder.

OK, let's make it static and extend the API.

> 
> > void report_skip(const char *msg_fmt, bool pass, bool skip, ...)
> > 
> > Why not do that?
> 
> Yeah, some cases want to unconditionally skip, so we'd want to have
> both.  I'll think of naming during lunch :)
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH kvm-unit-tests v2 00/12] Improve the output of test runners

[Andrew Jones]

On Fri, Dec 18, 2015 at 01:38:31PM +0100, Radim Krčmář wrote:
> 2015-12-17 14:04-0600, Andrew Jones:
> > On Thu, Dec 17, 2015 at 06:53:31PM +0100, Radim Krčmář wrote:
> >> v1: http://www.spinics.net/lists/kvm/msg125202.html
> >> 
> >> Drew brought up the existence of scripts/mkstandalone.sh, which
> >> significantly expanded v2 (and my set of curses) ...
> >> I didn't want to do the same twice, so first part of this series,
> >> [1-4/12], reuses run() from run_tests.sh and does some non-conservative
> >> changes to scripts/mkstandalone.sh.  scripts/mkstandalone.sh is lacking
> >> behind run_tests.sh, but should be good enough to fulfill its purpose.
> >> 
> >> The output of run_tests.sh has also changed a bit and now looks like
> >> this (you'll again need to imagine colours):
> >> 
> >> > PASS apic (14 tests)
> >> > PASS ioapic (19 tests)
> >> > PASS smptest (1 tests)
> >> > PASS smptest3 (1 tests)
> >> > PASS vmexit_cpuid
> >> > PASS vmexit_vmcall
> > 
> > Why do some tests, which have only 1 test, say (1 tests), but other
> > tests don't say anything?
> 
> Some tests don't use lib/report, hence don't print "^SUMMARY:", so we
> don't really want to know what happens there.
> 
> > Some nice improvements with this series. I'm not sure I like depending
> > on bash in standalone tests, but then that could just be cause I worked
> > pretty hard at avoiding the dependency, and thus I'll have to cry at the
> > loss of it...
> 
> Knowing the percentage of KVM+QEMU installations without bash would
> help.  I expect it to be very close to zero, which makes compassion hard
> to find ... sorry.

I'll look at the mkstandalone changes again. If taking a bash dependency
to maintain our sanity is necessary, then I won't fight it.

> 
> > Please review the series I'll send in about 2 minutes, so we can discuss
> > how to integrate them.
> 
> Will do, thanks.  (Please excuse the delay.)
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html