Re: [Qemu-devel] [PATCH RFC v5 5/5] qemu-iotests: s390x: fix test 051

[Max Reitz]

On 2015-02-10 at 03:00, Xiao Guang Chen wrote:
> The tests for device type "ide_cd" should only be tested for the pc platform.
> The default device id of hard disk on the s390 platform differs to that
> of the x86 platform. A new variable device_id is defined and "virtio0"
> set for the s390 platform. A x86 platform specific output file is also
> needed.
> A new filter was added to filter s390 warnings.
>
> Signed-off-by: Xiao Guang Chen,address@hidden
> ---
>   tests/qemu-iotests/051           |  79 +++++---
>   tests/qemu-iotests/051.out       | 160 +++++----------
>   tests/qemu-iotests/051.pc.out    | 427 +++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/common.filter |   7 +
>   4 files changed, 532 insertions(+), 141 deletions(-)
>   create mode 100644 tests/qemu-iotests/051.pc.out
>
> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> index 11c858f..fedf2d4 100755
> --- a/tests/qemu-iotests/051
> +++ b/tests/qemu-iotests/051

[snip]

> @@ -241,28 +253,37 @@ echo
>   echo === Snapshot mode ===
>   echo
>   
> +case "$QEMU_DEFAULT_MACHINE" in
> +    pc)
> +        device_id="ide0-hd0"
> +        ;;
> +    *)
> +        device_id="virtio0"

"virtio0" is available for s390-virtio by default, but probably not for 
other platforms. So I suggest you change the * into s390-virtio and skip 
these tests for non-s390-virtio and non-pc platforms.

On second thought, this test is probably highly platform-specific 
(because the reference output is tied pretty tightly to the default 
device types of the platform in used). Maybe we need a 
_supported_platforms or something, and in this case only pc and 
s390-virtio would be supported.

However, considering that the iotests only worked for x86 so far, I'd be 
fine with you just leaving this patch completely as-is (it does not 
break x86, and I trust you that it makes the test work on s390) and we 
care about making it work (or just making sure it gets skipped) for 
other platforms later on.

Therefore, with the "Signed-off-by" line fixed:

Reviewed-by: Max Reitz <address@hidden>

I have further comments below concerning the issue of other platforms 
than pc and s390-virtio, but they don't influence this R-b.

> +        ;;
> +esac
> +
>   $QEMU_IO -c "write -P 0x11 0 4k" "$TEST_IMG" | _filter_qemu_io
>   
> -echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive file="$TEST_IMG" -snapshot | _filter_qemu_io
> -echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file="$TEST_IMG",snapshot=on | _filter_qemu_io
> -echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file.filename="$TEST_IMG",driver=qcow2,snapshot=on | _filter_qemu_io
> -echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file.filename="$TEST_IMG",driver=qcow2 -snapshot | _filter_qemu_io
> -echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file="file:$TEST_IMG" -snapshot | _filter_qemu_io
> -echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file="file:$TEST_IMG",snapshot=on | _filter_qemu_io
> +echo "qemu-io $device_id \"write -P 0x22 0 4k\"" | run_qemu -drive 
> file="$TEST_IMG" -snapshot | _filter_qemu_io
> +echo "qemu-io $device_id \"write -P 0x22 0 4k\"" | run_qemu -drive 
> file="$TEST_IMG",snapshot=on | _filter_qemu_io
> +echo "qemu-io $device_id \"write -P 0x22 0 4k\"" | run_qemu -drive 
> file.filename="$TEST_IMG",driver=qcow2,snapshot=on | _filter_qemu_io
> +echo "qemu-io $device_id \"write -P 0x22 0 4k\"" | run_qemu -drive 
> file.filename="$TEST_IMG",driver=qcow2 -snapshot | _filter_qemu_io
> +echo "qemu-io $device_id \"write -P 0x22 0 4k\"" | run_qemu -drive 
> file="file:$TEST_IMG" -snapshot | _filter_qemu_io
> +echo "qemu-io $device_id \"write -P 0x22 0 4k\"" | run_qemu -drive 
> file="file:$TEST_IMG",snapshot=on | _filter_qemu_io
>   
>   # Opening a read-only file r/w with snapshot=on
>   chmod u-w "$TEST_IMG"
> -echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file="$TEST_IMG" -snapshot | _filter_qemu_io
> -echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive 
> file="$TEST_IMG",snapshot=on | _filter_qemu_io
> +echo "qemu-io $device_id \"write -P 0x22 0 4k\"" | run_qemu -drive 
> file="$TEST_IMG" -snapshot | _filter_qemu_io
> +echo "qemu-io $device_id \"write -P 0x22 0 4k\"" | run_qemu -drive 
> file="$TEST_IMG",snapshot=on | _filter_qemu_io
>   chmod u+w "$TEST_IMG"
>   
>   $QEMU_IO -c "read -P 0x11 0 4k" "$TEST_IMG" | _filter_qemu_io
>   
> -echo 'qemu-io ide0-hd0 "write -P 0x22 0 4k"' | run_qemu -drive file="$TEST_IMG",snapshot=off | _filter_qemu_io
> +echo "qemu-io $device_id \"write -P 0x22 0 4k\"" | run_qemu -drive 
> file="$TEST_IMG",snapshot=off | _filter_qemu_io
>   
>   $QEMU_IO -c "read -P 0x22 0 4k" "$TEST_IMG" | _filter_qemu_io
>   
> -echo -e 'qemu-io ide0-hd0 "write -P 0x33 0 4k"\ncommit ide0-hd0' | run_qemu -drive file="$TEST_IMG",snapshot=on | _filter_qemu_io
> +echo -e "qemu-io $device_id \"write -P 0x33 0 4k\"\ncommit $device_id" | run_qemu -drive 
> file="$TEST_IMG",snapshot=on | _filter_qemu_io
>   
>   $QEMU_IO -c "read -P 0x33 0 4k" "$TEST_IMG" | _filter_qemu_io

[snip]

> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index 06e1bb0..1e26c05 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -154,9 +154,16 @@ _filter_qemu_io()
>           -e "s/qemu-io> //g"
>   }
>   
> +# removes s390 warnings
> +_filter_s390()
> +{
> +    sed -e '/^(qemu) Warning: Orphaned drive without device:.*$/d'
> +}
> +

Hm, I'd rather call it "_filter_orphan" or something because it doesn't 
look to me like this is really s390-specific (we might see the same 
warning on other platforms, too).

Apart from that, I think you should not filter out the (qemu) part (if 
possible) because that is not part of the warning. Since the reference 
output is now used for all non-PC platforms, imagine there being a 
non-s390 platform where this warning does not appear; it will output the 
(qemu) prompt but it won't be filtered and thus the output will be 
different.

>   # replace occurrences of QEMU_PROG with "qemu"
>   _filter_qemu()
>   {
> +    _filter_s390 | \
>       sed -e "s#\\(^\\|(qemu) \\)$(basename $QEMU_PROG):#\1QEMU_PROG:#" \
>           -e 's#^QEMU [0-9]\+\.[0-9]\+\.[0-9]\+ monitor#QEMU X.Y.Z monitor#' \
>           -e '/main-loop: WARNING: I\/O thread spun for [0-9]\+ iterations/d' \

To simulate a non-s390 platform to test these general reference outputs 
against, you can just use an x86 host and force default_machine to some 
arbitrary value (like "foobar"). This will result in the tests not using 
the PC-specific reference outputs and code paths, and thus give you 
another platform to test these supposedly general reference outputs against.

Max