| Message ID | 1456772003-27911-4-git-send-email-drjones@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/02/2016 19:53, Andrew Jones wrote: > +exit_fixup () Can you rename the function to run_qemu or something like that? Otherwise it's not clear that it actually runs "$@". > + errors=$("${@}" 2>&1 1>&${stdout} | tee >(cat - 1>&2); exit ${PIPESTATUS[0]}) Whoa! :) So stdout goes to {stdout} and from there to the real stdout; stderr instead is tee'd to stderr and $errors. Is the "tee" and "cat" necessary? Can you just use printf somewhat like: # stdout to {stdout}, stderr to $errors errors=$("$@" 2>&1 1>&${stdout}) ret=$? printf '%s\n' "$errors" >&2 ? Or something like that (for example I'm not sure if you need the \n). In either case, stdout and stderr can be mixed. > + temp_file RUNTIME_arch_run <(echo "#!/bin/bash"; cat scripts/arch-run.bash "$TEST_DIR/run") Please use a pipe instead of a <(...): (echo "#! /bin/bash" cat scripts/arch-run.bash "$TEST_DIR/run") | tempfile RUNTIME_arch_run by changing this in temp_file: local file="$2" gzip - < $file to local file="${2--}" gzip -c "$file" Paolo -- 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
2016-03-01 22:29+0100, Paolo Bonzini: > On 29/02/2016 19:53, Andrew Jones wrote: >> + errors=$("${@}" 2>&1 1>&${stdout} | tee >(cat - 1>&2); exit ${PIPESTATUS[0]}) > > Whoa! :) So stdout goes to {stdout} and from there to the real stdout; > stderr instead is tee'd to stderr and $errors. Yeah. :) > Is the "tee" and "cat" necessary? Can you just use printf somewhat like: They are, if you want want to improve chances of getting the original order of stdout and stderr. > # stdout to {stdout}, stderr to $errors > errors=$("$@" 2>&1 1>&${stdout}) > ret=$? > printf '%s\n' "$errors" >&2 (Previous version did this any my suggestion was to change it ...) > ? Or something like that (for example I'm not sure if you need the \n). Trying to improve the original solution, What about a bit more understandable errors=$("${@}" 2> >(tee >(cat) >&2) >&$stdout) ? Which gets rid of some redirections and PIPESTATUS. > In either case, stdout and stderr can be mixed. Yes, we can't have a good solution here. >> + temp_file RUNTIME_arch_run <(echo "#!/bin/bash"; cat scripts/arch-run.bash "$TEST_DIR/run") > > Please use a pipe instead of a <(...): (Is it for aesthetic reasons?) Thanks. -- 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
On 02/03/2016 13:57, Radim Kr?má? wrote: > 2016-03-01 22:29+0100, Paolo Bonzini: >> On 29/02/2016 19:53, Andrew Jones wrote: >>> + errors=$("${@}" 2>&1 1>&${stdout} | tee >(cat - 1>&2); exit ${PIPESTATUS[0]}) >> >> Whoa! :) So stdout goes to {stdout} and from there to the real stdout; >> stderr instead is tee'd to stderr and $errors. > > Yeah. :) > >> Is the "tee" and "cat" necessary? Can you just use printf somewhat like: > > They are, if you want want to improve chances of getting the original > order of stdout and stderr. Getting the original order can't really work if you use separate file descriptions. I'm not sure how you would have something on stderr, except on the very last line? The idea being that 1) the test only writes to stdout 2) QEMU only writes to stderr 3) QEMU exits after writing to stderr. If this is true, the proposed assignment/printf pair works just fine. >>> + temp_file RUNTIME_arch_run <(echo "#!/bin/bash"; cat scripts/arch-run.bash "$TEST_DIR/run") >> >> Please use a pipe instead of a <(...): > > (Is it for aesthetic reasons?) Yes, nothing more. Also, gzip has "-c", might as well use it. Paolo -- 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
2016-03-02 15:44+0100, Paolo Bonzini: > On 02/03/2016 13:57, Radim Kr?má? wrote: >> 2016-03-01 22:29+0100, Paolo Bonzini: >>> On 29/02/2016 19:53, Andrew Jones wrote: >>>> + errors=$("${@}" 2>&1 1>&${stdout} | tee >(cat - 1>&2); exit ${PIPESTATUS[0]}) >>> >>> Whoa! :) So stdout goes to {stdout} and from there to the real stdout; >>> stderr instead is tee'd to stderr and $errors. >> >> Yeah. :) >> >>> Is the "tee" and "cat" necessary? Can you just use printf somewhat like: >> >> They are, if you want want to improve chances of getting the original >> order of stdout and stderr. > > Getting the original order can't really work if you use separate file > descriptions. Yes, there is no "true" order ... my concern is about transparency: tee delays stderr much less than if we buffered stderr until QEMU has finished, so mixed stdout/stderr has better chance of being visible in sans-wrapper order. > I'm not sure how you would have something on stderr, except on the very > last line? The idea being that 1) the test only writes to stdout 2) > QEMU only writes to stderr 3) QEMU exits after writing to stderr. If > this is true, the proposed assignment/printf pair works just fine. QEMU prints some warnings without immediately exiting, e.g. warning: host doesn't support requested feature: CPUID.01H:ECX.vmx [bit 5] So it's possible that stdout is printed after stderr. Preserving the visible output could be less confusing in these rare cases. I agree with the late printf if you think that the ugly code is not worth it. -- 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
On 02/03/2016 16:42, Radim Kr?má? wrote: >> > I'm not sure how you would have something on stderr, except on the very >> > last line? The idea being that 1) the test only writes to stdout 2) >> > QEMU only writes to stderr 3) QEMU exits after writing to stderr. If >> > this is true, the proposed assignment/printf pair works just fine. > > QEMU prints some warnings without immediately exiting, e.g. > warning: host doesn't support requested feature: CPUID.01H:ECX.vmx [bit 5] > > So it's possible that stdout is printed after stderr. Preserving the > visible output could be less confusing in these rare cases. I agree > with the late printf if you think that the ugly code is not worth it. Perhaps we can add an "stderr:" line if $errors is not empty, so that it is clear that the order is not maintained. Paolo -- 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
2016-03-02 17:05+0100, Paolo Bonzini: > On 02/03/2016 16:42, Radim Kr?má? wrote: >>> > I'm not sure how you would have something on stderr, except on the very >>> > last line? The idea being that 1) the test only writes to stdout 2) >>> > QEMU only writes to stderr 3) QEMU exits after writing to stderr. If >>> > this is true, the proposed assignment/printf pair works just fine. >> >> QEMU prints some warnings without immediately exiting, e.g. >> warning: host doesn't support requested feature: CPUID.01H:ECX.vmx [bit 5] >> >> So it's possible that stdout is printed after stderr. Preserving the >> visible output could be less confusing in these rare cases. I agree >> with the late printf if you think that the ugly code is not worth it. > > Perhaps we can add an "stderr:" line if $errors is not empty, so that it > is clear that the order is not maintained. I prefer to print stderr afterwards without any separator. ("stderr:\n" taints stderr and the information provided by it isn't that hard to figure out from the output or code. I think that this line would attract attention, because it is an unexpected element, and therefore waste more time than silently reordering the output. The line might be useful when we merge stdout and stderr into one stream, but that doesn't happen at this point.) -- 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
diff --git a/arm/run b/arm/run index dc0a33204c20b..c9463de6dd241 100755 --- a/arm/run +++ b/arm/run @@ -6,6 +6,7 @@ if [ -z "$STANDALONE" ]; then exit 2 fi source config.mak + source scripts/arch-run.bash fi processor="$PROCESSOR" @@ -71,7 +72,4 @@ command="$qemu $M -cpu $processor $chr_testdev" command+=" -display none -serial stdio -kernel" echo $command "$@" -$command "$@" -ret=$? -echo Return value from qemu: $ret -exit $ret +exit_fixup $command "$@" diff --git a/powerpc/run b/powerpc/run index 45492a1cb8afc..8ac684cc7c12f 100755 --- a/powerpc/run +++ b/powerpc/run @@ -6,6 +6,7 @@ if [ -z "$STANDALONE" ]; then exit 2 fi source config.mak + source scripts/arch-run.bash fi if [ -c /dev/kvm ]; then @@ -46,10 +47,22 @@ command="$qemu $M -bios $FIRMWARE" command+=" -display none -serial stdio -kernel" echo $command "$@" -#FIXME: rtas-poweroff always exits with zero, so we have to parse -# the true exit code from the output. -lines=$($command "$@") +# powerpc tests currently exit with rtas-poweroff, which exits with 0. +# exit_fixup treats that as a failure exit and returns 1, so we need +# to fixup the fixup below by parsing the true exit code from the output. +# The second fixup is also a FIXME, because once we add chr-testdev +# support for powerpc, we won't need the second fixup. +lines=$(exit_fixup $command "$@") +ret=$? echo "$lines" -ret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/') -echo Return value from qemu: $ret +if [ $ret -eq 1 ]; then + testret=$(grep '^EXIT: ' <<<"$lines" | sed 's/.*STATUS=\([0-9][0-9]*\).*/\1/') + if [ "$testret" ]; then + if [ $testret -eq 1 ]; then + ret=0 + else + ret=$testret + fi + fi +fi exit $ret diff --git a/scripts/arch-run.bash b/scripts/arch-run.bash new file mode 100644 index 0000000000000..3a63ed179a7de --- /dev/null +++ b/scripts/arch-run.bash @@ -0,0 +1,61 @@ +############################################################################## +# exit_fixup translates the ambiguous exit status in Table1 to that in Table2. +# Table3 simply documents the complete status table. +# +# Table1: Before fixup +# -------------------- +# 0 - Unexpected exit from QEMU (possible signal), or the unittest did +# not use debug-exit +# 1 - most likely unittest succeeded, or QEMU failed +# +# Table2: After fixup +# ------------------- +# 0 - Everything succeeded +# 1 - most likely QEMU failed +# +# Table3: Complete table +# ---------------------- +# 0 - SUCCESS +# 1 - most likely QEMU failed +# 2 - most likely a run script failed +# 3 - most likely the unittest failed +# 127 - most likely the unittest called abort() +# 1..127 - FAILURE (could be QEMU, a run script, or the unittest) +# >= 128 - Signal (signum = status - 128) +############################################################################## +exit_fixup () +{ + local stdout errors ret sig + + exec {stdout}>&1 + errors=$("${@}" 2>&1 1>&${stdout} | tee >(cat - 1>&2); exit ${PIPESTATUS[0]}) + ret=$? + exec {stdout}>&- + + if [ "$errors" ]; then + sig=$(grep 'terminating on signal' <<<"$errors") + if [ "$sig" ]; then + sig=$(sed 's/.*terminating on signal \([0-9][0-9]*\).*/\1/' <<<"$sig") + fi + fi + + if [ $ret -eq 0 ]; then + # Some signals result in a zero return status, but the + # error log tells the truth. + if [ "$sig" ]; then + ((ret=sig+128)) + else + # Exiting with zero (non-debugexit) is an error + ret=1 + fi + elif [ $ret -eq 1 ]; then + # Even when ret==1 (unittest success) if we also got stderr + # logs, then we assume a QEMU failure. Otherwise we translate + # status of 1 to 0 (SUCCESS) + if [ -z "$errors" ]; then + ret=0 + fi + fi + + return $ret +} diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh index 408bb05480b11..4c201aa9a4743 100755 --- a/scripts/mkstandalone.sh +++ b/scripts/mkstandalone.sh @@ -61,7 +61,7 @@ generate_test () temp_file bin "$kernel" args[3]='$bin' - temp_file RUNTIME_arch_run "$TEST_DIR/run" + temp_file RUNTIME_arch_run <(echo "#!/bin/bash"; cat scripts/arch-run.bash "$TEST_DIR/run") cat scripts/runtime.bash diff --git a/scripts/runtime.bash b/scripts/runtime.bash index 04adb9d070f13..9c973f26f8de6 100644 --- a/scripts/runtime.bash +++ b/scripts/runtime.bash @@ -48,7 +48,7 @@ function run() eval $cmdline ret=$? - if [ $ret -le 1 ]; then + if [ $ret -eq 0 ]; then echo -e "\e[32mPASS\e[0m $1" else echo -e "\e[31mFAIL\e[0m $1" diff --git a/x86/README b/x86/README index 996ed33546d62..218fe1a1c6f1a 100644 --- a/x86/README +++ b/x86/README @@ -41,7 +41,9 @@ Tests in this directory and what they do: pcid: basic functionality test of PCID/INVPCID feature Legacy notes: - The exit status of the binary (and the script) is inconsistent: with - qemu-system, after the unit-test is done, the exit status of qemu is 1, - different from the 'old style' qemu-kvm, whose exit status in successful - completion is 0. + The exit status of the binary is inconsistent; with qemu-system, after + the unit-test is done, the exit status of qemu is 1, different from the + 'old style' qemu-kvm, whose exit status in successful completion is 0. + The run script converts the qemu-system exit status to 0 (SUCCESS), and + treats the legacy exit status of 0 as an error, converting it to an exit + status of 1. diff --git a/x86/run b/x86/run index 22d7f2cad0d06..b07c815c5a6e1 100755 --- a/x86/run +++ b/x86/run @@ -1,5 +1,7 @@ #!/bin/bash +[ -z "$STANDALONE" ] && source scripts/arch-run.bash + qemubinarysearch="${QEMU:-qemu-kvm qemu-system-x86_64}" for qemucmd in ${qemubinarysearch} @@ -43,7 +45,4 @@ fi command="${qemu} -enable-kvm $pc_testdev -vnc none -serial stdio $pci_testdev $hyperv_testdev -kernel" echo ${command} "$@" -${command} "$@" -ret=$? -echo Return value from qemu: $ret -exit $ret +exit_fixup ${command} "$@"