[PATCH kvm-unit-tests 0/6] Improve the output of test runners

[PATCH kvm-unit-tests 0/6] Improve the output of test runners

[Radim Krčmář]

This series is a mix of patches that change the output of run_tests.sh
and x86-run.  The output of ./run_tests.sh now looks like this:

> PASS apic (14 tests, 0 unexpected failures)
> PASS ioapic (19 tests, 0 unexpected failures)
> PASS smptest (1 tests, 0 unexpected failures)
> PASS smptest3 (1 tests, 0 unexpected failures)
> 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, 0 unexpected failures)
> skip pku (0 tests, 0 unexpected failures)
> PASS emulator (132 tests, 0 unexpected failures, 1 skipped)
> PASS eventinj (13 tests, 0 unexpected failures)
> PASS hypercall (2 tests, 0 unexpected failures)
> PASS idt_test (4 tests, 0 unexpected failures)
> PASS msr (13 tests, 0 unexpected failures)
> PASS pmu (67 tests, 0 unexpected failures, 1 expected failures)
> PASS port80 
> PASS realmode 
> PASS s3 
> PASS sieve 
> PASS tsc (3 tests, 0 unexpected failures)
> PASS tsc_adjust (5 tests, 0 unexpected failures)
> PASS xsave (17 tests, 0 unexpected failures)
> PASS rmap_chain 
> skip svm (0 tests, 0 unexpected failures)
> skip svm-disabled (0 tests, 0 unexpected failures)
> skip taskswitch (i386 only)
> skip taskswitch2 (i386 only)
> PASS kvmclock_test 
> PASS pcid (3 tests, 0 unexpected failures)
> skip vmx (0 tests, 0 unexpected failures)
> PASS debug (7 tests, 0 unexpected failures)
> qemu-kvm: Property '.hv-synic' not found
> skip hyperv_synic (failed $(echo quit | $qemu -enable-kvm -cpu kvm64,hv_synic -device hyperv-testdev -monitor stdio > /dev/null))

I don't like it too much, but it still seems like an improvement.

The main difference is in extra information for tests using
report_summary(), that smap and pku tests don't fail, pmu test isn't
completely skipped, and svm, vmx, and hyperv_synic are skipped.

The old one looked like this:

> PASS apic
> PASS ioapic
> PASS smptest
> PASS smptest3
> 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
> FAIL smap
> FAIL pku
> PASS emulator
> PASS eventinj
> PASS hypercall
> PASS idt_test
> PASS msr
> skip pmu (/proc/sys/kernel/nmi_watchdog not equal to 0)
> PASS port80
> PASS realmode
> PASS s3
> PASS sieve
> PASS tsc
> PASS tsc_adjust
> PASS xsave
> PASS rmap_chain
> PASS svm
> PASS svm-disabled
> skip taskswitch (i386 only)
> skip taskswitch2 (i386 only)
> PASS kvmclock_test
> PASS pcid
> PASS vmx
> PASS debug
> qemu-kvm: Property '.hv-synic' not found
> PASS hyperv_synic


Radim Krčmář (6):
  lib/report: allow test skipping
  x86/*: report that the test was skipped
  x86/pmu: expect failure with nmi_watchdog
  run_tests: generalize check
  x86/hyperv_synic: check for support before testing
  run_tests: print summary

 lib/libcflat.h     |  1 +
 lib/report.c       | 43 +++++++++++++++++++++++++++++--------------
 run_tests.sh       | 35 ++++++++++++++++++-----------------
 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 ++--
 12 files changed, 68 insertions(+), 45 deletions(-)

-- 
2.6.4

[PATCH kvm-unit-tests 1/6] lib/report: allow test skipping

[Radim Krčmář]

This patch allows us to 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 report()s were done, but
the test still ended with report_summary().

When the whole test is skipped, ./run_tests.sh prints "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>
---
 lib/libcflat.h |  1 +
 lib/report.c   | 43 +++++++++++++++++++++++++++++--------------
 run_tests.sh   | 13 ++++++++-----
 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..e07baa347298 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,27 @@ void report_prefix_pop(void)
 	spin_unlock(&lock);
 }
 
-void va_report_xfail(const char *msg_fmt, bool xfail, bool cond, va_list va)
+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 +73,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 +81,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 +99,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/run_tests.sh b/run_tests.sh
index fad22a935b00..4d813b9a7084 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -55,12 +55,15 @@ function run()
     # extra_params in the config file may contain backticks that need to be
     # expanded, so use eval to start qemu
     eval $cmdline >> test.log
+    # The first bit of return value is too hard to use, just skip it.
+    # Unit-tests' return value is shifted by one.
+    case $(($? >> 1)) in
+    0)  echo -ne "\e[32mPASS\e[0m" ;;
+    77) echo -ne "skip" ;;
+    *)  echo -ne "\e[31mFAIL\e[0m"
+    esac
 
-    if [ $? -le 1 ]; then
-        echo -e "\e[32mPASS\e[0m $1"
-    else
-        echo -e "\e[31mFAIL\e[0m $1"
-    fi
+    echo " $1"
 }
 
 function usage()
-- 
2.6.4

[PATCH kvm-unit-tests 2/6] 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>
---
 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..57af86de8f8c 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\n");
     }
 }
 
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 3/6] 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>
---
 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..4ca93235b977 100644
--- a/x86/pmu.c
+++ b/x86/pmu.c
@@ -92,6 +92,7 @@ struct pmu_event {
 };
 
 static int num_counters;
+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 4/6] run_tests: 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>
---
 run_tests.sh | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/run_tests.sh b/run_tests.sh
index 4d813b9a7084..b0b064f2e341 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -35,17 +35,10 @@ 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 $check || {
+        echo "skip $1 (failed \$($check))"
+        return
+    }
 
     cmdline="TESTNAME=$testname ACCEL=$accel ./$TEST_DIR-run $kernel -smp $smp $opts"
     if [ $verbose != 0 ]; then
-- 
2.6.4

[PATCH kvm-unit-tests 5/6] 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.

x86/hyperv_synic is "skip" instead of "PASS", without KVM/QEMU support.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 Probably not the nicest solution around ...
 "dry_run_qemu_and_skip_if_it_fails=true" parameter in unittest.cfg
 might be better.

 x86/unittests.cfg | 1 +
 1 file changed, 1 insertion(+)

diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 6b94ad93dcf0..32fc5f7037e1 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 -enable-kvm -cpu kvm64,hv_synic -device hyperv-testdev -monitor stdio > /dev/null
-- 
2.6.4

[PATCH kvm-unit-tests 6/6] run_tests: print summary

[Radim Krčmář]

Add shortened SUMMARY line into the output (in parentheses).

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

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 run_tests.sh | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/run_tests.sh b/run_tests.sh
index b0b064f2e341..d0b282f7b079 100755
--- a/run_tests.sh
+++ b/run_tests.sh
@@ -21,6 +21,7 @@ function run()
     local arch="$6"
     local check="$7"
     local accel="$8"
+    local output
 
     if [ -z "$testname" ]; then
         return
@@ -47,7 +48,7 @@ function run()
 
     # extra_params in the config file may contain backticks that need to be
     # expanded, so use eval to start qemu
-    eval $cmdline >> test.log
+    output=`eval $cmdline`
     # The first bit of return value is too hard to use, just skip it.
     # Unit-tests' return value is shifted by one.
     case $(($? >> 1)) in
@@ -56,7 +57,11 @@ function run()
     *)  echo -ne "\e[31mFAIL\e[0m"
     esac
 
-    echo " $1"
+    echo -n " $1 "
+
+    printf "%s\n" "$output" |
+      tee -a test.log |
+      sed 'x;$s/^SUMMARY: //;ta;$s/.*//p;d;:a s/^/(/;s/$/)/'
 }
 
 function usage()
-- 
2.6.4

Re: [PATCH kvm-unit-tests 1/6] lib/report: allow test skipping

[Andrew Jones]

On Mon, Dec 14, 2015 at 10:24:16PM +0100, Radim Krčmář wrote:
> This patch allows us to 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 report()s were done, but
> the test still ended with report_summary().
> 
> When the whole test is skipped, ./run_tests.sh prints "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>
> ---
>  lib/libcflat.h |  1 +
>  lib/report.c   | 43 +++++++++++++++++++++++++++++--------------
>  run_tests.sh   | 13 ++++++++-----
>  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..e07baa347298 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,27 @@ void report_prefix_pop(void)
>  	spin_unlock(&lock);
>  }
>  
> -void va_report_xfail(const char *msg_fmt, bool xfail, bool cond, va_list va)
> +void va_report(const char *msg_fmt, bool pass, bool xfail, bool skip, va_list va)

Line greater than 80 char here. Yes, that was supposed to induce an eye
roll. But... this file doesn't have any "long" lines yet, so we could
continue avoiding them.

>  {
> -	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 +73,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 +81,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 +99,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/run_tests.sh b/run_tests.sh
> index fad22a935b00..4d813b9a7084 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -55,12 +55,15 @@ function run()
>      # extra_params in the config file may contain backticks that need to be
>      # expanded, so use eval to start qemu
>      eval $cmdline >> test.log
> +    # The first bit of return value is too hard to use, just skip it.
> +    # Unit-tests' return value is shifted by one.
> +    case $(($? >> 1)) in
> +    0)  echo -ne "\e[32mPASS\e[0m" ;;
> +    77) echo -ne "skip" ;;

Why not "\e[31mSKIP\e[0m"? (and without those escape sequences echo doesn't
need -e)

> +    *)  echo -ne "\e[31mFAIL\e[0m"
> +    esac
>  
> -    if [ $? -le 1 ]; then
> -        echo -e "\e[32mPASS\e[0m $1"
> -    else
> -        echo -e "\e[31mFAIL\e[0m $1"
> -    fi
> +    echo " $1"
>  }
>  
>  function usage()
> -- 
> 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

Thanks,
drew

Re: [PATCH kvm-unit-tests 3/6] x86/pmu: expect failure with nmi_watchdog

[Andrew Jones]

On Mon, Dec 14, 2015 at 10:24:18PM +0100, Radim Krčmář wrote:
> 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>
> ---
>  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..4ca93235b977 100644
> --- a/x86/pmu.c
> +++ b/x86/pmu.c
> @@ -92,6 +92,7 @@ struct pmu_event {
>  };
>  
>  static int num_counters;
> +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);

How about outputting "host_nmi_watchdog=%d" as well?

>  }
>  
>  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
> 
> --
> 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 2/6] x86/*: report skipped tests

[Andrew Jones]

On Mon, Dec 14, 2015 at 10:24:17PM +0100, Radim Krčmář wrote:
> 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>
> ---
>  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..57af86de8f8c 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\n");

You probably don't want this '\n' anymore.

>      }
>  }
>  
> 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
> 
> --
> 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 4/6] run_tests: generalize check

[Andrew Jones]

On Mon, Dec 14, 2015 at 10:24:19PM +0100, Radim Krčmář wrote:
> config attribute "check" is currently unused.
> Provide a simple implementation instead of removing it.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  run_tests.sh | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/run_tests.sh b/run_tests.sh
> index 4d813b9a7084..b0b064f2e341 100755
> --- a/run_tests.sh
> +++ b/run_tests.sh
> @@ -35,17 +35,10 @@ 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 $check || {
> +        echo "skip $1 (failed \$($check))"
> +        return
> +    }

I think we should use "\e[33mSKIP\e[0m" for skip. Maybe we should create
pass(),fail(),skip() functions in order to make sure all callers use the
same prefix with the same color.

>  
>      cmdline="TESTNAME=$testname ACCEL=$accel ./$TEST_DIR-run $kernel -smp $smp $opts"
>      if [ $verbose != 0 ]; then
> -- 
> 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 1/6] lib/report: allow test skipping

[Andrew Jones]

On Mon, Dec 14, 2015 at 04:00:19PM -0600, Andrew Jones wrote:
> On Mon, Dec 14, 2015 at 10:24:16PM +0100, Radim Krčmář wrote:
> > This patch allows us to 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 report()s were done, but
> > the test still ended with report_summary().
> > 
> > When the whole test is skipped, ./run_tests.sh prints "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>
> > ---
> >  lib/libcflat.h |  1 +
> >  lib/report.c   | 43 +++++++++++++++++++++++++++++--------------
> >  run_tests.sh   | 13 ++++++++-----
> >  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..e07baa347298 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,27 @@ void report_prefix_pop(void)
> >  	spin_unlock(&lock);
> >  }
> >  
> > -void va_report_xfail(const char *msg_fmt, bool xfail, bool cond, va_list va)
> > +void va_report(const char *msg_fmt, bool pass, bool xfail, bool skip, va_list va)
> 
> Line greater than 80 char here. Yes, that was supposed to induce an eye
> roll. But... this file doesn't have any "long" lines yet, so we could
> continue avoiding them.
> 
> >  {
> > -	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 +73,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 +81,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 +99,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/run_tests.sh b/run_tests.sh
> > index fad22a935b00..4d813b9a7084 100755
> > --- a/run_tests.sh
> > +++ b/run_tests.sh
> > @@ -55,12 +55,15 @@ function run()
> >      # extra_params in the config file may contain backticks that need to be
> >      # expanded, so use eval to start qemu
> >      eval $cmdline >> test.log
> > +    # The first bit of return value is too hard to use, just skip it.
> > +    # Unit-tests' return value is shifted by one.
> > +    case $(($? >> 1)) in
> > +    0)  echo -ne "\e[32mPASS\e[0m" ;;
> > +    77) echo -ne "skip" ;;
> 
> Why not "\e[31mSKIP\e[0m"? (and without those escape sequences echo doesn't
> need -e)

oops, copy+paste error, I meant to put use color code 33 (yellow).

> 
> > +    *)  echo -ne "\e[31mFAIL\e[0m"
> > +    esac
> >  
> > -    if [ $? -le 1 ]; then
> > -        echo -e "\e[32mPASS\e[0m $1"
> > -    else
> > -        echo -e "\e[31mFAIL\e[0m $1"
> > -    fi
> > +    echo " $1"
> >  }
> >  
> >  function usage()
> > -- 
> > 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
> 
> Thanks,
> drew

Re: [PATCH kvm-unit-tests 0/6] Improve the output of test runners

[Andrew Jones]

On Mon, Dec 14, 2015 at 10:24:15PM +0100, Radim Krčmář wrote:
> This series is a mix of patches that change the output of run_tests.sh
> and x86-run.  The output of ./run_tests.sh now looks like this:
> 
> > PASS apic (14 tests, 0 unexpected failures)
> > PASS ioapic (19 tests, 0 unexpected failures)
> > PASS smptest (1 tests, 0 unexpected failures)
> > PASS smptest3 (1 tests, 0 unexpected failures)
> > 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, 0 unexpected failures)
> > skip pku (0 tests, 0 unexpected failures)
> > PASS emulator (132 tests, 0 unexpected failures, 1 skipped)
> > PASS eventinj (13 tests, 0 unexpected failures)
> > PASS hypercall (2 tests, 0 unexpected failures)
> > PASS idt_test (4 tests, 0 unexpected failures)
> > PASS msr (13 tests, 0 unexpected failures)
> > PASS pmu (67 tests, 0 unexpected failures, 1 expected failures)
> > PASS port80 
> > PASS realmode 
> > PASS s3 
> > PASS sieve 
> > PASS tsc (3 tests, 0 unexpected failures)
> > PASS tsc_adjust (5 tests, 0 unexpected failures)
> > PASS xsave (17 tests, 0 unexpected failures)
> > PASS rmap_chain 
> > skip svm (0 tests, 0 unexpected failures)
> > skip svm-disabled (0 tests, 0 unexpected failures)
> > skip taskswitch (i386 only)
> > skip taskswitch2 (i386 only)
> > PASS kvmclock_test 
> > PASS pcid (3 tests, 0 unexpected failures)
> > skip vmx (0 tests, 0 unexpected failures)
> > PASS debug (7 tests, 0 unexpected failures)
> > qemu-kvm: Property '.hv-synic' not found
> > skip hyperv_synic (failed $(echo quit | $qemu -enable-kvm -cpu kvm64,hv_synic -device hyperv-testdev -monitor stdio > /dev/null))

I'm not sure I like the (summary) addition. A summary of why we skip
would be useful, like the (i386 only) stuff, but otherwise it doesn't
seem necessary, and the "(failed $(echo quit | $qemu -enable-kvm -cpu
kvm64,hv_synic -device hyperv-testdev -monitor stdio > /dev/null))"
summary is a bit ugly, wrapping on many terminals.

Another comment that is series wide is that all these changes need to be
tested with 'make standalone' (and many of your patches will require
changes to scripts/mkstandalone.sh, for which you will never forgive
me for having written :-)

Thanks,
drew

Re: [PATCH kvm-unit-tests 0/6] Improve the output of test runners

[Paolo Bonzini]

On 14/12/2015 22:24, Radim Krčmář wrote:
> This series is a mix of patches that change the output of run_tests.sh
> and x86-run.  The output of ./run_tests.sh now looks like this:

I like the idea, thanks!  I agree with Andrew about pretty much
everything, except that I like having the summary close to PASS/FAIL.

>> qemu-kvm: Property '.hv-synic' not found
>> skip hyperv_synic (failed $(echo quit | $qemu -enable-kvm -cpu kvm64,hv_synic -device hyperv-testdev -monitor stdio > /dev/null))

Here I'd just print "failed check".

Paolo

Re: [PATCH kvm-unit-tests 1/6] lib/report: allow test skipping

[Radim Krčmář]

2015-12-14 16:12-0600, Andrew Jones:
> On Mon, Dec 14, 2015 at 04:00:19PM -0600, Andrew Jones wrote:
>> On Mon, Dec 14, 2015 at 10:24:16PM +0100, Radim Krčmář wrote:
>> > ---
>> > diff --git a/lib/libcflat.h b/lib/libcflat.h
>> > -void va_report_xfail(const char *msg_fmt, bool xfail, bool cond, va_list va)
>> > +void va_report(const char *msg_fmt, bool pass, bool xfail, bool skip, va_list va)
>> 
>> Line greater than 80 char here. Yes, that was supposed to induce an eye
>> roll. But... this file doesn't have any "long" lines yet, so we could
>> continue avoiding them.

Ah, yeah, young'uns these days ...

>> > diff --git a/run_tests.sh b/run_tests.sh
>> > index fad22a935b00..4d813b9a7084 100755
>> > --- a/run_tests.sh
>> > +++ b/run_tests.sh
>> > @@ -55,12 +55,15 @@ function run()
>> >      # extra_params in the config file may contain backticks that need to be
>> >      # expanded, so use eval to start qemu
>> >      eval $cmdline >> test.log
>> > +    # The first bit of return value is too hard to use, just skip it.
>> > +    # Unit-tests' return value is shifted by one.
>> > +    case $(($? >> 1)) in
>> > +    0)  echo -ne "\e[32mPASS\e[0m" ;;
>> > +    77) echo -ne "skip" ;;
>> 
>> Why not "\e[31mSKIP\e[0m"? (and without those escape sequences echo doesn't
>> need -e)
> 
> oops, copy+paste error, I meant to put use color code 33 (yellow).

Will do.  (I started with yellow SKIP, but then kept white skip as I
didn't research the reason why it was chosen.)

Re: [PATCH kvm-unit-tests 2/6] x86/*: report skipped tests

[Radim Krčmář]

2015-12-14 16:07-0600, Andrew Jones:
> On Mon, Dec 14, 2015 at 10:24:17PM +0100, Radim Krčmář wrote:
>> 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>
>> ---
>> diff --git a/x86/apic.c b/x86/apic.c
>> @@ -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\n");
> 
> You probably don't want this '\n' anymore.

Thanks, I should have written that no care at all was given :(

Re: [PATCH kvm-unit-tests 3/6] x86/pmu: expect failure with nmi_watchdog

[Radim Krčmář]

2015-12-14 16:05-0600, Andrew Jones:
> On Mon, Dec 14, 2015 at 10:24:18PM +0100, Radim Krčmář wrote:
>> diff --git a/x86/pmu.c b/x86/pmu.c
>> @@ -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);
> 
> How about outputting "host_nmi_watchdog=%d" as well?

It's already implied in the output.  Prefix will be XPASS/XFAIL if
host_nmi_watchdog=1 and PASS/FAIL otherwise.

Should it still be explicitly printed?

Re: [PATCH kvm-unit-tests 4/6] run_tests: generalize check

[Radim Krčmář]

2015-12-14 16:11-0600, Andrew Jones:
> On Mon, Dec 14, 2015 at 10:24:19PM +0100, Radim Krčmář wrote:
> > ---
>> diff --git a/run_tests.sh b/run_tests.sh
>> @@ -35,17 +35,10 @@ 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 $check || {
>> +        echo "skip $1 (failed \$($check))"
>> +        return
>> +    }
> 
> I think we should use "\e[33mSKIP\e[0m" for skip. Maybe we should create
> pass(),fail(),skip() functions in order to make sure all callers use the
> same prefix with the same color.

Sounds good.

Re: [PATCH kvm-unit-tests 0/6] Improve the output of test runners

[Radim Krčmář]

2015-12-14 16:20-0600, Andrew Jones:
> On Mon, Dec 14, 2015 at 10:24:15PM +0100, Radim Krčmář wrote:
>> > skip vmx (0 tests, 0 unexpected failures)
>> > PASS debug (7 tests, 0 unexpected failures)
>> > qemu-kvm: Property '.hv-synic' not found
>> > skip hyperv_synic (failed $(echo quit | $qemu -enable-kvm -cpu kvm64,hv_synic -device hyperv-testdev -monitor stdio > /dev/null))
> 
> I'm not sure I like the (summary) addition. A summary of why we skip
> would be useful, like the (i386 only) stuff, but otherwise it doesn't
> seem necessary, and the "(failed $(echo quit | $qemu -enable-kvm -cpu
> kvm64,hv_synic -device hyperv-testdev -monitor stdio > /dev/null))"
> summary is a bit ugly, wrapping on many terminals.

The summary should help to make callers more certain of what happened
without spending time with tests.log.
I'd like the summary more if we didn't print "0 unexpected failures" in
passing/skipped cases, so maybe we can agree on that?

I'll shorten the failed case.

> Another comment that is series wide is that all these changes need to be
> tested with 'make standalone' (and many of your patches will require
> changes to scripts/mkstandalone.sh, for which you will never forgive
> me for having written :-)

First time I heard about it. :)

Re: [PATCH kvm-unit-tests 0/6] Improve the output of test runners

[Radim Krčmář]

2015-12-15 11:19+0100, Paolo Bonzini:
> On 14/12/2015 22:24, Radim Krčmář wrote:
>> This series is a mix of patches that change the output of run_tests.sh
>> and x86-run.  The output of ./run_tests.sh now looks like this:
> 
> I like the idea, thanks!  I agree with Andrew about pretty much
> everything, except that I like having the summary close to PASS/FAIL.

I'm planning a summary without useless informaton in v2, which might be
acceptable for everyone :)

>>> qemu-kvm: Property '.hv-synic' not found
>>> skip hyperv_synic (failed $(echo quit | $qemu -enable-kvm -cpu kvm64,hv_synic -device hyperv-testdev -monitor stdio > /dev/null))
> 
> Here I'd just print "failed check".

Ok, thanks.

Re: [PATCH kvm-unit-tests 3/6] x86/pmu: expect failure with nmi_watchdog

[Andrew Jones]

On Tue, Dec 15, 2015 at 02:01:01PM +0100, Radim Krčmář wrote:
> 2015-12-14 16:05-0600, Andrew Jones:
> > On Mon, Dec 14, 2015 at 10:24:18PM +0100, Radim Krčmář wrote:
> >> diff --git a/x86/pmu.c b/x86/pmu.c
> >> @@ -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);
> > 
> > How about outputting "host_nmi_watchdog=%d" as well?
> 
> It's already implied in the output.  Prefix will be XPASS/XFAIL if
> host_nmi_watchdog=1 and PASS/FAIL otherwise.
> 
> Should it still be explicitly printed?

I think it could help interpret the results without needing to read
the code, but these types of tests generally require reading the
code...

drew

> --
> 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 0/6] Improve the output of test runners

[Andrew Jones]

On Tue, Dec 15, 2015 at 02:13:25PM +0100, Radim Krčmář wrote:
> 2015-12-15 11:19+0100, Paolo Bonzini:
> > On 14/12/2015 22:24, Radim Krčmář wrote:
> >> This series is a mix of patches that change the output of run_tests.sh
> >> and x86-run.  The output of ./run_tests.sh now looks like this:
> > 
> > I like the idea, thanks!  I agree with Andrew about pretty much
> > everything, except that I like having the summary close to PASS/FAIL.
> 
> I'm planning a summary without useless informaton in v2, which might be
> acceptable for everyone :)

Yeah, I can live that :-)

drew