Re: [Qemu-devel] [PATCH RFC v7 6/7] qemu-iotests: s390x: fix test 051

[Max Reitz <mreitz@redhat.com>]

Hi Tu Bo :-)

On 27.04.2015 05:57, tu bo wrote:
> Hi Max:
>
> On 04/24/2015 01:00 AM, Max Reitz wrote:
>> On 23.04.2015 04:42, 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 orphan warnings.
>>>
>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>> Reviewed-by: Michael Mueller <mimu@linux.vnet.ibm.com>
>>> Signed-off-by: Xiao Guang Chen <chenxg@linux.vnet.ibm.com>
>>> ---
>>>   tests/qemu-iotests/051           |  79 ++++---
>>>   tests/qemu-iotests/051.out       | 156 +++++---------
>>>   tests/qemu-iotests/051.pc.out    | 433 
>>> +++++++++++++++++++++++++++++++++++++++
>>>   tests/qemu-iotests/common.filter |   7 +
>>>   4 files changed, 545 insertions(+), 130 deletions(-)
>>>   create mode 100644 tests/qemu-iotests/051.pc.out
>>>
>>> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
>>> index 0360f37..8de16a5 100755
>>> --- a/tests/qemu-iotests/051
>>> +++ b/tests/qemu-iotests/051
>>> @@ -147,13 +147,19 @@ run_qemu -drive if=ide
>>>   run_qemu -drive if=virtio
>>>   run_qemu -drive if=scsi
>>>   -run_qemu -drive if=none,id=disk -device ide-cd,drive=disk
>>> -run_qemu -drive if=none,id=disk -device lsi53c895a -device 
>>> scsi-cd,drive=disk
>>> -
>>> -run_qemu -drive if=none,id=disk -device ide-drive,drive=disk
>>> -run_qemu -drive if=none,id=disk -device ide-hd,drive=disk
>>> -run_qemu -drive if=none,id=disk -device lsi53c895a -device 
>>> scsi-disk,drive=disk
>>> -run_qemu -drive if=none,id=disk -device lsi53c895a -device 
>>> scsi-hd,drive=disk
>>> +case "$QEMU_DEFAULT_MACHINE" in
>>> +    pc)
>>> +        run_qemu -drive if=none,id=disk -device ide-cd,drive=disk
>>> +        run_qemu -drive if=none,id=disk -device lsi53c895a -device 
>>> scsi-cd,drive=disk
>>> +
>>> +        run_qemu -drive if=none,id=disk -device ide-drive,drive=disk
>>> +        run_qemu -drive if=none,id=disk -device ide-hd,drive=disk
>>> +        run_qemu -drive if=none,id=disk -device lsi53c895a -device 
>>> scsi-disk,drive=disk
>>> +        run_qemu -drive if=none,id=disk -device lsi53c895a -device 
>>> scsi-hd,drive=disk
>>> +        ;;
>>> +    *)
>>> +        ;;
>>> +esac
>>>     echo
>>>   echo === Read-only ===
>>> @@ -167,13 +173,19 @@ run_qemu -drive 
>>> file="$TEST_IMG",if=ide,readonly=on
>>>   run_qemu -drive file="$TEST_IMG",if=virtio,readonly=on
>>>   run_qemu -drive file="$TEST_IMG",if=scsi,readonly=on
>>>   -run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
>>> -device ide-cd,drive=disk
>>> -run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
>>> -device lsi53c895a -device scsi-cd,drive=disk
>>> +case "$QEMU_DEFAULT_MACHINE" in
>>> +    pc)
>>> +        run_qemu -drive 
>>> file="$TEST_IMG",if=none,id=disk,readonly=on -device ide-cd,drive=disk
>>> +        run_qemu -drive 
>>> file="$TEST_IMG",if=none,id=disk,readonly=on -device lsi53c895a 
>>> -device scsi-cd,drive=disk
>>>   -run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
>>> -device ide-drive,drive=disk
>>> -run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
>>> -device ide-hd,drive=disk
>>> -run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
>>> -device lsi53c895a -device scsi-disk,drive=disk
>>> -run_qemu -drive file="$TEST_IMG",if=none,id=disk,readonly=on 
>>> -device lsi53c895a -device scsi-hd,drive=disk
>>> +        run_qemu -drive 
>>> file="$TEST_IMG",if=none,id=disk,readonly=on -device 
>>> ide-drive,drive=disk
>>> +        run_qemu -drive 
>>> file="$TEST_IMG",if=none,id=disk,readonly=on -device ide-hd,drive=disk
>>> +        run_qemu -drive 
>>> file="$TEST_IMG",if=none,id=disk,readonly=on -device lsi53c895a 
>>> -device scsi-disk,drive=disk
>>> +        run_qemu -drive 
>>> file="$TEST_IMG",if=none,id=disk,readonly=on -device lsi53c895a 
>>> -device scsi-hd,drive=disk
>>> +        ;;
>>> +    *)
>>> +        ;;
>>> +esac
>>>     echo
>>>   echo === Cache modes ===
>>> @@ -182,12 +194,12 @@ echo
>>>   # Cannot use the test image because cache=none might not work on 
>>> the host FS
>>>   # Use cdrom so that we won't get errors about missing media
>>>   -run_qemu -drive media=cdrom,cache=none
>>> -run_qemu -drive media=cdrom,cache=directsync
>>> -run_qemu -drive media=cdrom,cache=writeback
>>> -run_qemu -drive media=cdrom,cache=writethrough
>>> -run_qemu -drive media=cdrom,cache=unsafe
>>> -run_qemu -drive media=cdrom,cache=invalid_value
>>> +run_qemu -drive if=scsi,media=cdrom,cache=none
>>> +run_qemu -drive if=scsi,media=cdrom,cache=directsync
>>> +run_qemu -drive if=scsi,media=cdrom,cache=writeback
>>> +run_qemu -drive if=scsi,media=cdrom,cache=writethrough
>>> +run_qemu -drive if=scsi,media=cdrom,cache=unsafe
>>> +run_qemu -drive if=scsi,media=cdrom,cache=invalid_value
>>>     echo
>>>   echo === Specifying the protocol layer ===
>>> @@ -251,28 +263,37 @@ echo
>>>   echo === Snapshot mode ===
>>>   echo
>>>   +case "$QEMU_DEFAULT_MACHINE" in
>>> +    pc)
>>> +        device_id="ide0-hd0"
>>> +        ;;
>>> +    s390)
>>> +        device_id="virtio0"
>>> +        ;;
>>> +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
>>>   diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
>>> index 2890eac..63e0e81 100644
>>> --- a/tests/qemu-iotests/051.out
>>> +++ b/tests/qemu-iotests/051.out
>>> @@ -5,16 +5,16 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT 
>>> size=134217728 backing_file='TEST_DIR
>>>   === Unknown option ===
>>>     Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=
>>> -QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=: 
>>> Block format 'qcow2' used by device 'ide0-hd0' doesn't support the 
>>> option 'unknown_opt'
>>> +QEMU_PROG: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=: 
>>> Block format 'qcow2' used by device 'virtio0' doesn't support the 
>>> option 'unknown_opt'
>>>     Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on
>>> -QEMU_PROG: -drive 
>>> file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on: Block format 
>>> 'qcow2' used by device 'ide0-hd0' doesn't support the option 
>>> 'unknown_opt'
>>> +QEMU_PROG: -drive 
>>> file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=on: Block format 
>>> 'qcow2' used by device 'virtio0' doesn't support the option 
>>> 'unknown_opt'
>>>     Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234
>>> -QEMU_PROG: -drive 
>>> file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234: Block format 
>>> 'qcow2' used by device 'ide0-hd0' doesn't support the option 
>>> 'unknown_opt'
>>> +QEMU_PROG: -drive 
>>> file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=1234: Block format 
>>> 'qcow2' used by device 'virtio0' doesn't support the option 
>>> 'unknown_opt'
>>>     Testing: -drive file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo
>>> -QEMU_PROG: -drive 
>>> file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo: Block format 
>>> 'qcow2' used by device 'ide0-hd0' doesn't support the option 
>>> 'unknown_opt'
>>> +QEMU_PROG: -drive 
>>> file=TEST_DIR/t.qcow2,format=qcow2,unknown_opt=foo: Block format 
>>> 'qcow2' used by device 'virtio0' doesn't support the option 
>>> 'unknown_opt'
>>>       === Unknown protocol option ===
>>> @@ -51,8 +51,7 @@ QEMU_PROG: -drive 
>>> file=TEST_DIR/t.qcow2,driver=qcow2,format=qcow2: Cannot specif
>>>     Testing: -device virtio-scsi-pci -device scsi-hd
>>>   QEMU X.Y.Z monitor - type 'help' for more information
>>> -(qemu) QEMU_PROG: -device scsi-hd: drive property not set
>>> -QEMU_PROG: -device scsi-hd: Device 'scsi-hd' could not be initialized
>>> +(qemu) QEMU_PROG: -device virtio-scsi-pci: No 'PCI' bus found for 
>>> device 'virtio-scsi-pci'
>>
>> This change looks like you'd rather just disable this test for 
>> non-PCI systems. Also, it's a change from v6 which forces me to 
>> remind you to please remove Reviewed-by statements from the commit 
>> message if there are functional changes in a patch from the 
>> previously reviewed version.
>>
>> (Because just like in this case, the reviewer (i.e., me) may find 
>> some of these changes questionable, or might even outright object them.)
>>
>> Max
> 1. Sure. will remove Reviewed-by statements if any functional changes 
> in a new patch version. thanks for the reminder

Thank you :-)

> 2. Test 051 will be executed for PCI systems like x86,  and test 051 
> will be executed for non-PCI systems like s390x as well. However, its 
> golden test data is different(051.out is for s390x,  051.pc.out is for 
> x86)
>     Test steps are different between PCI systems and non-PCI systems. 
> For instance, the tests for device type "ide_cd" should only be tested 
> for the pc platform, and be disabled for non-PCI systems.

Right. So I think the same is the case here: A PCI device 
(virtio-scsi-pci) is created by the test, so it should be disabled for 
non-PCI systems. While testing against the reference output "No 'PCI' 
bus found" will probably not break on non-PCI systems, it doesn't seem 
like it's something we want to test here, though.

Max