[kvm-unit-tests PATCH 0/3] run_tests.sh changes

[kvm-unit-tests PATCH 0/3] run_tests.sh changes

[Andrew Jones]

Improve exit code handling and add unittest timeout support.

Andrew Jones (3):
  run_tests.sh: reduce return code ambiguity
  cleanup unittests.cfg headers
  add timeout support

 arm/run                 |  8 ++++++--
 arm/unittests.cfg       | 27 ++++++++++++++++++---------
 run_tests.sh            | 32 +++++++++++++++++++++++++++++---
 scripts/functions.bash  |  8 ++++++--
 scripts/mkstandalone.sh | 34 ++++++++++++++++++++++++++++------
 x86/run                 |  8 ++++++--
 x86/unittests.cfg       | 22 +++++++++++++++-------
 7 files changed, 108 insertions(+), 31 deletions(-)

-- 
2.4.3

[kvm-unit-tests PATCH 1/3] run_tests.sh: reduce return code ambiguity

[Andrew Jones]

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

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

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

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

This patch attempts to reduce that ambiguity, by also looking at stderr.
With it, we have

 0      - unexpected exit from qemu, or the unittest not using debug-exit.
          Consider it a FAILURE
 1      - unittest SUCCESS
 < 128  - something failed (could be the unittest, qemu, or a run script)
          Check the logs.
 >= 128 - signal (signum = code - 128)

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 run_tests.sh            | 27 +++++++++++++++++++++++++--
 scripts/mkstandalone.sh | 27 ++++++++++++++++++++++-----
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/run_tests.sh b/run_tests.sh
index fad22a935b007..f8de08cfb21b5 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 errlog sig ret
 
     if [ -z "$testname" ]; then
         return
@@ -54,10 +55,32 @@ 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
+    errlog=$(mktemp)
+    eval $cmdline >> test.log 2> $errlog
+    ret=$?
+
+    if [ "$(stat -c %s $errlog)" != "0" ]; then
+        # Some signals result in a zero return code, but the error log
+        # tells the truth.
+        sig="$(grep 'terminating on signal' $errlog | sed 's/.*terminating on signal \([0-9][0-9]*\).*/\1/')"
+        if [ $ret -eq 0 ] && [ "$sig" ]; then
+            ((ret=sig+128))
+        elif [ $ret -eq 1 ]; then
+            # We got the unittest SUCCESS code, but also error messages,
+            # let's assume qemu failed.
+            ret=2
+        fi
+        cat $errlog >> test.log
+    fi
+    rm -f $errlog
 
-    if [ $? -le 1 ]; then
+    if [ $ret -eq 0 ]; then
+        echo -e "\e[31mFAIL\e[0m $1 (debug-exit not called)"
+    elif [ $ret -eq 1 ]; then
         echo -e "\e[32mPASS\e[0m $1"
+    elif [ $ret -ge 128 ]; then
+        ((sig=ret-128))
+        echo -e "\e[31mFAIL\e[0m $1 (got signal $sig)"
     else
         echo -e "\e[31mFAIL\e[0m $1"
     fi
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index 3ce244aff67b9..94ea0467c5be6 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -83,8 +83,9 @@ exit 1
 EOF
 else
 	cat <<EOF >> $standalone
-trap 'rm -f \$bin; exit 1' HUP INT TERM
+trap 'rm -f \$bin \$errlog; exit 1' HUP INT TERM
 bin=\`mktemp\`
+errlog=\`mktemp\`
 base64 -d << 'BIN_EOF' | zcat > \$bin &&
 EOF
 gzip - < $kernel | base64 >> $standalone
@@ -109,16 +110,32 @@ else
 	done
 
 	cmdline="\`echo '$cmdline' | sed s%$kernel%\$bin%\`"
-	\$qemu \$cmdline -smp $smp $opts
+	\$qemu \$cmdline -smp $smp $opts 2> \$errlog
 	ret=\$?
+	echo Return value from qemu: \$ret
+
+	if [ "\`stat -c %s \$errlog\`" != "0" ]; then
+		sig="\`grep 'terminating on signal' \$errlog | sed 's/.*terminating on signal \([0-9][0-9]*\).*/\1/'\`"
+		if [ \$ret -eq 0 ] && [ "\$sig" ]; then
+			ret=\`expr \$sig + 128\`
+		elif [ \$ret -eq 1 ]; then
+			ret=2
+		fi
+		cat \$errlog
+	fi
 fi
-echo Return value from qemu: \$ret
-if [ \$ret -le 1 ]; then
+
+if [ \$ret -eq 0 ]; then
+	echo "FAIL $testname (debug-exit not called)" 1>&2
+elif [ \$ret -eq 1 ]; then
 	echo PASS $testname 1>&2
+elif [ \$ret -ge 128 ]; then
+	echo "FAIL $testname (got signal \`expr \$ret - 128\`)" 1>&2
 else
 	echo FAIL $testname 1>&2
 fi
-rm -f \$bin
+
+rm -f \$bin \$errlog
 exit 0
 EOF
 fi
-- 
2.4.3

[kvm-unit-tests PATCH 2/3] cleanup unittests.cfg headers

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arm/unittests.cfg | 26 +++++++++++++++++---------
 x86/unittests.cfg | 21 ++++++++++++++-------
 2 files changed, 31 insertions(+), 16 deletions(-)

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

[kvm-unit-tests PATCH 3/3] add timeout support

[Andrew Jones]

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arm/run                 | 8 ++++++--
 arm/unittests.cfg       | 1 +
 run_tests.sh            | 5 ++++-
 scripts/functions.bash  | 8 ++++++--
 scripts/mkstandalone.sh | 9 +++++++--
 x86/run                 | 8 ++++++--
 x86/unittests.cfg       | 1 +
 7 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/arm/run b/arm/run
index 4a648697d7fb5..a66892becc8ae 100755
--- a/arm/run
+++ b/arm/run
@@ -75,10 +75,14 @@ chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
 M+=",accel=$ACCEL"
 command="$qemu $M -cpu $processor $chr_testdev"
 command+=" -display none -serial stdio -kernel"
-echo $command "$@"
+
+if [ "$TIMEOUT" ]; then
+	timeout_cmd="timeout --foreground $TIMEOUT"
+fi
+echo $timeout_cmd $command "$@"
 
 if [ "$DRYRUN" != "yes" ]; then
-	$command "$@"
+	$timeout_cmd $command "$@"
 	ret=$?
 	echo Return value from qemu: $ret
 	exit $ret
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 926bbb153728b..8c6c475f050fc 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -15,6 +15,7 @@
 # accel = kvm|tcg		# Optionally specify if test must run with
 #				# kvm or tcg. If not specified, then kvm will
 #				# be used when available.
+# timeout = <duration>		# Optionally specify a timeout.
 ##############################################################################
 
 #
diff --git a/run_tests.sh b/run_tests.sh
index f8de08cfb21b5..23494fa032c49 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 timeout="${9:-$TIMEOUT}"
     local errlog sig ret
 
     if [ -z "$testname" ]; then
@@ -48,7 +49,7 @@ function run()
         fi
     done
 
-    cmdline="TESTNAME=$testname ACCEL=$accel ./$TEST_DIR-run $kernel -smp $smp $opts"
+    cmdline="TESTNAME=$testname ACCEL=$accel TIMEOUT=$timeout ./$TEST_DIR-run $kernel -smp $smp $opts"
     if [ $verbose != 0 ]; then
         echo $cmdline
     fi
@@ -78,6 +79,8 @@ function run()
         echo -e "\e[31mFAIL\e[0m $1 (debug-exit not called)"
     elif [ $ret -eq 1 ]; then
         echo -e "\e[32mPASS\e[0m $1"
+    elif [ $ret -eq 124 ]; then
+        echo -e "\e[31mFAIL\e[0m $1 (timeout; duration=$timeout)"
     elif [ $ret -ge 128 ]; then
         ((sig=ret-128))
         echo -e "\e[31mFAIL\e[0m $1 (got signal $sig)"
diff --git a/scripts/functions.bash b/scripts/functions.bash
index f13fe6f88f23d..ee9143c5d630d 100644
--- a/scripts/functions.bash
+++ b/scripts/functions.bash
@@ -11,12 +11,13 @@ function for_each_unittest()
 	local arch
 	local check
 	local accel
+	local timeout
 
 	exec {fd}<"$unittests"
 
 	while read -u $fd line; do
 		if [[ "$line" =~ ^\[(.*)\]$ ]]; then
-			"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel"
+			"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
 			testname=${BASH_REMATCH[1]}
 			smp=1
 			kernel=""
@@ -25,6 +26,7 @@ function for_each_unittest()
 			arch=""
 			check=""
 			accel=""
+			timeout=""
 		elif [[ $line =~ ^file\ *=\ *(.*)$ ]]; then
 			kernel=$TEST_DIR/${BASH_REMATCH[1]}
 		elif [[ $line =~ ^smp\ *=\ *(.*)$ ]]; then
@@ -39,8 +41,10 @@ function for_each_unittest()
 			check=${BASH_REMATCH[1]}
 		elif [[ $line =~ ^accel\ *=\ *(.*)$ ]]; then
 			accel=${BASH_REMATCH[1]}
+		elif [[ $line =~ ^timeout\ *=\ *(.*)$ ]]; then
+			timeout=${BASH_REMATCH[1]}
 		fi
 	done
-	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel"
+	"$cmd" "$testname" "$groups" "$smp" "$kernel" "$opts" "$arch" "$check" "$accel" "$timeout"
 	exec {fd}<&-
 }
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index 94ea0467c5be6..80eb269b430c6 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -30,6 +30,7 @@ function mkstandalone()
 	local arch="$6"
 	local check="$7"
 	local accel="$8"
+	local timeout="$9"
 
 	if [ -z "$testname" ]; then
 		return 1
@@ -97,8 +98,12 @@ if [ "\$QEMU" ]; then
 	qemu="\$QEMU"
 fi
 
+if [ "$timeout" ]; then
+	timeout_cmd='timeout --foreground $timeout'
+fi
+
 MAX_SMP="MAX_SMP"
-echo \$qemu $cmdline -smp $smp $opts
+echo \$timeout_cmd \$qemu $cmdline -smp $smp $opts
 
 cmdline="\`echo '$cmdline' | sed s%$kernel%_NO_FILE_4Uhere_%\`"
 if \$qemu \$cmdline 2>&1 | grep 'No accelerator found'; then
@@ -110,7 +115,7 @@ else
 	done
 
 	cmdline="\`echo '$cmdline' | sed s%$kernel%\$bin%\`"
-	\$qemu \$cmdline -smp $smp $opts 2> \$errlog
+	\$timeout_cmd \$qemu \$cmdline -smp $smp $opts 2> \$errlog
 	ret=\$?
 	echo Return value from qemu: \$ret
 
diff --git a/x86/run b/x86/run
index 39a7cb92115cb..b82fea17cb040 100755
--- a/x86/run
+++ b/x86/run
@@ -43,10 +43,14 @@ else
 fi
 
 command="${qemu} -enable-kvm $pc_testdev -vnc none -serial stdio $pci_testdev $hyperv_testdev -kernel"
-echo ${command} "$@"
+
+if [ "$TIMEOUT" ]; then
+	timeout_cmd="timeout --foreground $TIMEOUT"
+fi
+echo $timeout_cmd ${command} "$@"
 
 if [ "$DRYRUN" != "yes" ]; then
-	${command} "$@"
+	$timeout_cmd ${command} "$@"
 	ret=$?
 	echo Return value from qemu: $ret
 	exit $ret
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index ae41781b5b72b..929521d3634a5 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -12,6 +12,7 @@
 #					# specific to only one.
 # groups = <group_name1> <group_name2> ...	# Used to identify test cases
 #						# with run_tests -g ...
+# timeout = <duration>		# Optionally specify a timeout.
 ##############################################################################
 
 [apic]
-- 
2.4.3

Re: [kvm-unit-tests PATCH 1/3] run_tests.sh: reduce return code ambiguity

[Radim Krčmář]

2015-12-17 14:10-0600, Andrew Jones:
> qemu/unittest exit codes are convoluted, causing codes 0 and 1
> to be ambiguous. Here are the possible meanings
> 
>  .-----------------------------------------------------------------.
>  |                | 0                                 | 1          |
>  |-----------------------------------------------------------------|
>  | QEMU           | did something successfully,       | FAILURE    |
>  |                | but probably didn't run the       |            |
>  |                | unittest, OR caught SIGINT,       |            |
>  |                | SIGHUP, or SIGTERM                |            |
>  |-----------------------------------------------------------------|
>  | unittest       | for some reason exited using      | SUCCESS    |
>  |                | ACPI/PSCI, not with debug-exit    |            |
>  .-----------------------------------------------------------------.
> 
> As we can see above, an exit code of zero is even ambiguous for each
> row, i.e. QEMU could exit with zero because it successfully completed,
> or because it caught a signal. unittest could exit with zero because
> it successfully powered-off, or because for some odd reason it powered-
> off instead of calling debug-exit.
> 
> And, the most fun is that exit-code == 1 means QEMU failed, but the
> unittest succeeded.
> 
> This patch attempts to reduce that ambiguity, by also looking at stderr.

Nice.

> With it, we have
> 
>  0      - unexpected exit from qemu, or the unittest not using debug-exit.
>           Consider it a FAILURE
>  1      - unittest SUCCESS
>  < 128  - something failed (could be the unittest, qemu, or a run script)
>           Check the logs.
>  >= 128 - signal (signum = code - 128)

I think this heuristic should be applied to {arm,x86}/run.
run_tests.sh would inherit it and we would finally get reasonable exit
values everywhere.

The resulting table would look like this:

 0     = unit-test passed
 77    = unit-test skipped (not implemented yet)
 124   = unit-test timeouted (implemented in [3/3])
 127   = qemu returned 0 (debug-exit probably wasn't called)
 > 128 = exited because of signal $? - 128
 *     = unit-test failed

(Signal 0 is not used, so we could map 128 to mean "debug-exit probably
 wasn't called", but others might not understand our signal convention.
 Anyway, it'd be best for us to start at 200, for `case $? in 2??)` ...)

> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> diff --git a/run_tests.sh b/run_tests.sh
> @@ -54,10 +55,32 @@ 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
> +    errlog=$(mktemp)
> +    eval $cmdline >> test.log 2> $errlog
| [...]
|      cat $errlog >> test.log

This assumes that stderr is always after stdout,

  eval $cmdline 2>&1 >> test.log | tee $errlog >> test.log

has a chance to print lines in wrong order too, but I think it's going
to be closer to the original.

Re: [kvm-unit-tests PATCH 3/3] add timeout support

[Radim Krčmář]

2015-12-17 14:10-0600, Andrew Jones:
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> diff --git a/arm/run b/arm/run
> @@ -75,10 +75,14 @@ chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
>  M+=",accel=$ACCEL"
>  command="$qemu $M -cpu $processor $chr_testdev"
>  command+=" -display none -serial stdio -kernel"
> -echo $command "$@"
> +
> +if [ "$TIMEOUT" ]; then
> +	timeout_cmd="timeout --foreground $TIMEOUT"

(command="timeout --foreground $TIMEOUT $command" # to keep the clutter
 down.)

> +fi

(We paste it three times, so I'd rather see this abstracted in a
 "scripts/" library.)

> diff --git a/run_tests.sh b/run_tests.sh
> @@ -21,6 +21,7 @@ function run()
> +    local timeout="${9:-$TIMEOUT}"
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> @@ -97,8 +98,12 @@ if [ "\$QEMU" ]; then
> +if [ "$timeout" ]; then
> +	timeout_cmd='timeout --foreground $timeout'

Both would be nicer if they took the TIMEOUT variable as an override.
We already don't do that for accel and the patch seems ok in other
regards,

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

Re: [kvm-unit-tests PATCH 1/3] run_tests.sh: reduce return code ambiguity

[Andrew Jones]

On Mon, Dec 21, 2015 at 05:31:24PM +0100, Radim Krčmář wrote:
> 2015-12-17 14:10-0600, Andrew Jones:
> > qemu/unittest exit codes are convoluted, causing codes 0 and 1
> > to be ambiguous. Here are the possible meanings
> > 
> >  .-----------------------------------------------------------------.
> >  |                | 0                                 | 1          |
> >  |-----------------------------------------------------------------|
> >  | QEMU           | did something successfully,       | FAILURE    |
> >  |                | but probably didn't run the       |            |
> >  |                | unittest, OR caught SIGINT,       |            |
> >  |                | SIGHUP, or SIGTERM                |            |
> >  |-----------------------------------------------------------------|
> >  | unittest       | for some reason exited using      | SUCCESS    |
> >  |                | ACPI/PSCI, not with debug-exit    |            |
> >  .-----------------------------------------------------------------.
> > 
> > As we can see above, an exit code of zero is even ambiguous for each
> > row, i.e. QEMU could exit with zero because it successfully completed,
> > or because it caught a signal. unittest could exit with zero because
> > it successfully powered-off, or because for some odd reason it powered-
> > off instead of calling debug-exit.
> > 
> > And, the most fun is that exit-code == 1 means QEMU failed, but the
> > unittest succeeded.
> > 
> > This patch attempts to reduce that ambiguity, by also looking at stderr.
> 
> Nice.
> 
> > With it, we have
> > 
> >  0      - unexpected exit from qemu, or the unittest not using debug-exit.
> >           Consider it a FAILURE
> >  1      - unittest SUCCESS
> >  < 128  - something failed (could be the unittest, qemu, or a run script)
> >           Check the logs.
> >  >= 128 - signal (signum = code - 128)
> 
> I think this heuristic should be applied to {arm,x86}/run.
> run_tests.sh would inherit it and we would finally get reasonable exit
> values everywhere.

Good idea. We can add the table to a scripts/functions.bash function and
use it everywhere.

> 
> The resulting table would look like this:
> 
>  0     = unit-test passed
>  77    = unit-test skipped (not implemented yet)
>  124   = unit-test timeouted (implemented in [3/3])
>  127   = qemu returned 0 (debug-exit probably wasn't called)

We already use 127 for abort(), called from a unit test, see
lib/abort.c. I guess we can use 126 for "debug-exit probably wasn't
called". We should also add a (unit test called abort) message for
127.

>  > 128 = exited because of signal $? - 128
>  *     = unit-test failed
> 
> (Signal 0 is not used, so we could map 128 to mean "debug-exit probably
>  wasn't called", but others might not understand our signal convention.

I think we want 128 to be the beginning of signal space, which goes all
the way up to 255, in order to allow exit code masking to work.

>  Anyway, it'd be best for us to start at 200, for `case $? in 2??)` ...)

Start what at 200? I think we have everything covered above. The mapping
looks like this

0	= success
1-63	= unit test failure code
64-127	= test suite failure code
128-255	= signal

which sounds good to me.

> 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> > diff --git a/run_tests.sh b/run_tests.sh
> > @@ -54,10 +55,32 @@ 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
> > +    errlog=$(mktemp)
> > +    eval $cmdline >> test.log 2> $errlog
> | [...]
> |      cat $errlog >> test.log
> 
> This assumes that stderr is always after stdout,

True. I'm not sure that matters when the unit test, which only uses stdout
will always output stuff serially with qemu, which could output a mix. But
your version below is fine by me if we want to pick up the need for the
pipe and tee.

> 
>   eval $cmdline 2>&1 >> test.log | tee $errlog >> test.log
> 
> has a chance to print lines in wrong order too, but I think it's going
> to be closer to the original.

I'll play with it and send a v2 soon.

Thanks,
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: [kvm-unit-tests PATCH 3/3] add timeout support

[Andrew Jones]

On Mon, Dec 21, 2015 at 06:04:20PM +0100, Radim Krčmář wrote:
> 2015-12-17 14:10-0600, Andrew Jones:
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> > diff --git a/arm/run b/arm/run
> > @@ -75,10 +75,14 @@ chr_testdev+=' -device virtconsole,chardev=ctd -chardev testdev,id=ctd'
> >  M+=",accel=$ACCEL"
> >  command="$qemu $M -cpu $processor $chr_testdev"
> >  command+=" -display none -serial stdio -kernel"
> > -echo $command "$@"
> > +
> > +if [ "$TIMEOUT" ]; then
> > +	timeout_cmd="timeout --foreground $TIMEOUT"
> 
> (command="timeout --foreground $TIMEOUT $command" # to keep the clutter
>  down.)
> 
> > +fi
> 
> (We paste it three times, so I'd rather see this abstracted in a
>  "scripts/" library.)

Sounds good

> 
> > diff --git a/run_tests.sh b/run_tests.sh
> > @@ -21,6 +21,7 @@ function run()
> > +    local timeout="${9:-$TIMEOUT}"
> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> > @@ -97,8 +98,12 @@ if [ "\$QEMU" ]; then
> > +if [ "$timeout" ]; then
> > +	timeout_cmd='timeout --foreground $timeout'
> 
> Both would be nicer if they took the TIMEOUT variable as an override.

Everything already takes TIMEOUT as an override, i.e.

TIMEOUT=3 ./run_tests.sh

and

TIMEOUT=3 arm/run arm/test.flat

will both already set a timeout for any test that didn't have a timeout
set in unittests.cfg, or wasn't run with run()/unittests.cfg. Or, did
you mean that you'd prefer TIMEOUT to override the timeout in
unittests.cfg? That does make some sense, in the case the one in the
config is longer than desired, however it also makes sense the way I
have it now when the one in the config is shorter than TIMEOUT (the
fallback default). I think I like it this way better.

> We already don't do that for accel and the patch seems ok in other
> regards,

Hmm, for accel I see a need for a patch allowing us to do

ACCEL=?? ./run_tests.sh

as I already have for TIMEOUT. Also, for both I should add a
mkstandalone patch allowing

TIMEOUT=? ACCEL=? make standalone

Thanks,
drew

> 
> Reviewed-by: Radim Krčmář <rkrcmar@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: [kvm-unit-tests PATCH 1/3] run_tests.sh: reduce return code ambiguity

[Radim Krčmář]

2015-12-21 13:35-0600, Andrew Jones:
> On Mon, Dec 21, 2015 at 05:31:24PM +0100, Radim Krčmář wrote:
> > 2015-12-17 14:10-0600, Andrew Jones:
>>  > 128 = exited because of signal $? - 128
>>  *     = unit-test failed
>> 
>> (Signal 0 is not used, so we could map 128 to mean "debug-exit probably
>>  wasn't called", but others might not understand our signal convention.
> 
> I think we want 128 to be the beginning of signal space, which goes all
> the way up to 255, in order to allow exit code masking to work.
> 
>>  Anyway, it'd be best for us to start at 200, for `case $? in 2??)` ...)
> 
> Start what at 200?

Signals, signal = $? - 200.  Shell default to decimal representation of
numbers, so using binary steps doesn't give an advantage and 55 is still
a plenty of space.  (I deplore elif cascade on the same variable, but we
can always convert the $? to binary/octal/hex, for `case` decoding. :])

>                    I think we have everything covered above. The mapping
> looks like this
> 
> 0	= success
> 1-63	= unit test failure code
> 64-127	= test suite failure code

77 is not that easy to categorize -- we want to return it from both.

> 128-255	= signal
> 
> which sounds good to me.

To me as well.

>> > Signed-off-by: Andrew Jones <drjones@redhat.com>
>> > ---
>> > diff --git a/run_tests.sh b/run_tests.sh
>> > @@ -54,10 +55,32 @@ 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
>> > +    errlog=$(mktemp)
>> > +    eval $cmdline >> test.log 2> $errlog
>> | [...]
>> |      cat $errlog >> test.log
>> 
>> This assumes that stderr is always after stdout,
> 
> True. I'm not sure that matters when the unit test, which only uses stdout
> will always output stuff serially with qemu, which could output a mix. But
> your version below is fine by me if we want to pick up the need for the
> pipe and tee.

Yeah, I assume that QEMU can warn during the test, or interact with its
own stdout in an ordered manner.  I don't think it matter much, but
there isn't a significant draw-back.

>>   eval $cmdline 2>&1 >> test.log | tee $errlog >> test.log
>> 
>> has a chance to print lines in wrong order too, but I think it's going
>> to be closer to the original.
> 
> I'll play with it and send a v2 soon.

Thanks, though I am quite distracted during the end of the year, so
"soon" won't be truly appreciated. :)

Re: [kvm-unit-tests PATCH 3/3] add timeout support

[Radim Krčmář]

2015-12-21 13:45-0600, Andrew Jones:
> On Mon, Dec 21, 2015 at 06:04:20PM +0100, Radim Krčmář wrote:
>> 2015-12-17 14:10-0600, Andrew Jones:
>> > diff --git a/run_tests.sh b/run_tests.sh
>> > @@ -21,6 +21,7 @@ function run()
>> > +    local timeout="${9:-$TIMEOUT}"
>> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
>> > @@ -97,8 +98,12 @@ if [ "\$QEMU" ]; then
>> > +if [ "$timeout" ]; then
>> > +	timeout_cmd='timeout --foreground $timeout'
>> 
>> Both would be nicer if they took the TIMEOUT variable as an override.
> 
> Everything already takes TIMEOUT as an override, i.e.
> 
> TIMEOUT=3 ./run_tests.sh
> 
> and
> 
> TIMEOUT=3 arm/run arm/test.flat
> 
> will both already set a timeout for any test that didn't have a timeout
> set in unittests.cfg, or wasn't run with run()/unittests.cfg.

Tests made with mkstandalone.sh ignore the TIMEOUT variable ...

>                                                               Or, did
> you mean that you'd prefer TIMEOUT to override the timeout in
> unittests.cfg?

... and yes, I think that we could have a
- global timeout for all tests.  Something far longer than any tests
  should take (2 minutes?).  To automatically handle random hangs.

- per-test timeout in unittests.cfg.  When the test is known to timeout
  often and the usual time to fail is much shorter than the global
  default.  (Shouldn't be used much.)

- TIMEOUT variable.  It has to override the global timeout and I think
  that if we are ever going to use it, it's because we want something
  weird.  Like using `TIMEOUT=0 ./run_tests.sh` to disable all
  timeouts, prolonging/shortening timeouts because of a special
  configuration, ...
  Because we should improve our defaults otherwise.

  (I'd probably allow something as evil as `eval`ing the TIMEOUT, for
   unlikely stuff like TIMEOUT='$(( ${timeout:-10} / 2 ))')

>                That does make some sense, in the case the one in the
> config is longer than desired, however it also makes sense the way I
> have it now when the one in the config is shorter than TIMEOUT (the
> fallback default). I think I like it this way better.

Ok, the difference was negligible to begin with.

>> We already don't do that for accel and the patch seems ok in other
>> regards,
> 
> Hmm, for accel I see a need for a patch allowing us to do
> 
> ACCEL=?? ./run_tests.sh

Btw. why do we have ACCEL when the project is *kvm*_unit_tests?

> as I already have for TIMEOUT. Also, for both I should add a
> mkstandalone patch allowing
> 
> TIMEOUT=? ACCEL=? make standalone

I'd also handle TIMEOUT/ACCEL in resulting standalone tests.

Re: [kvm-unit-tests PATCH 3/3] add timeout support

[Andrew Jones]

On Tue, Dec 22, 2015 at 07:02:21PM +0100, Radim Krčmář wrote:
> 2015-12-21 13:45-0600, Andrew Jones:
> > On Mon, Dec 21, 2015 at 06:04:20PM +0100, Radim Krčmář wrote:
> >> 2015-12-17 14:10-0600, Andrew Jones:
> >> > diff --git a/run_tests.sh b/run_tests.sh
> >> > @@ -21,6 +21,7 @@ function run()
> >> > +    local timeout="${9:-$TIMEOUT}"
> >> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> >> > @@ -97,8 +98,12 @@ if [ "\$QEMU" ]; then
> >> > +if [ "$timeout" ]; then
> >> > +	timeout_cmd='timeout --foreground $timeout'
> >> 
> >> Both would be nicer if they took the TIMEOUT variable as an override.
> > 
> > Everything already takes TIMEOUT as an override, i.e.
> > 
> > TIMEOUT=3 ./run_tests.sh
> > 
> > and
> > 
> > TIMEOUT=3 arm/run arm/test.flat
> > 
> > will both already set a timeout for any test that didn't have a timeout
> > set in unittests.cfg, or wasn't run with run()/unittests.cfg.
> 
> Tests made with mkstandalone.sh ignore the TIMEOUT variable ...
> 
> >                                                               Or, did
> > you mean that you'd prefer TIMEOUT to override the timeout in
> > unittests.cfg?
> 
> ... and yes, I think that we could have a
> - global timeout for all tests.  Something far longer than any tests
>   should take (2 minutes?).  To automatically handle random hangs.
> 
> - per-test timeout in unittests.cfg.  When the test is known to timeout
>   often and the usual time to fail is much shorter than the global
>   default.  (Shouldn't be used much.)
> 
> - TIMEOUT variable.  It has to override the global timeout and I think
>   that if we are ever going to use it, it's because we want something
>   weird.  Like using `TIMEOUT=0 ./run_tests.sh` to disable all
>   timeouts, prolonging/shortening timeouts because of a special
>   configuration, ...
>   Because we should improve our defaults otherwise.

OK, I'll do something allowing us to easily enable a long default
timeout.

> 
>   (I'd probably allow something as evil as `eval`ing the TIMEOUT, for
>    unlikely stuff like TIMEOUT='$(( ${timeout:-10} / 2 ))')

I'd prefer to avoid the evil^Weval stuff... And the timeout duration can
already be a floating point.

> 
> >                That does make some sense, in the case the one in the
> > config is longer than desired, however it also makes sense the way I
> > have it now when the one in the config is shorter than TIMEOUT (the
> > fallback default). I think I like it this way better.
> 
> Ok, the difference was negligible to begin with.
> 
> >> We already don't do that for accel and the patch seems ok in other
> >> regards,
> > 
> > Hmm, for accel I see a need for a patch allowing us to do
> > 
> > ACCEL=?? ./run_tests.sh
> 
> Btw. why do we have ACCEL when the project is *kvm*_unit_tests?

arm tests are sometimes tcg only. Hey, we'll take what we get for
arm, as we're sadly missing everything...

> 
> > as I already have for TIMEOUT. Also, for both I should add a
> > mkstandalone patch allowing
> > 
> > TIMEOUT=? ACCEL=? make standalone
> 
> I'd also handle TIMEOUT/ACCEL in resulting standalone tests.

OK

Thanks,
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