Re: [Qemu-devel] [PATCH v3 2/3] qemu-iotests: s390x: fix test 051

[Max Reitz]

On 01.12.2015 08:35, tu bo wrote:
> Hi Max:
> 
> 在 2015/12/1 3:38, Max Reitz 写道:
>> On 26.11.2015 10:53, Bo Tu wrote:
>>> From: Bo Tu <address@hidden>
>>>
>>> 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.
>>> Warning message expected for s390x when drive without device.
>>>
>>> Reviewed-by: Max Reitz <address@hidden>
>>
>> I can't remember having given this for this patch. ;-)
> 
> It's my mistake, please forgive me for it.

No problem.

>>> Reviewed-by: Sascha Silbe <address@hidden>
>>> Signed-off-by: Bo Tu <address@hidden>
>>> ---
>>>   tests/qemu-iotests/051          |  99 ++++++----
>>>   tests/qemu-iotests/051.out      | 422
>>> ----------------------------------------
>>>   tests/qemu-iotests/051.pc.out   | 422
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   tests/qemu-iotests/051.s390.out | 335 +++++++++++++++++++++++++++++++
>>>   tests/qemu-iotests/Makefile     |   7 +-
>>>   5 files changed, 828 insertions(+), 457 deletions(-)
>>>   delete mode 100644 tests/qemu-iotests/051.out
>>>   create mode 100644 tests/qemu-iotests/051.pc.out
>>>   create mode 100644 tests/qemu-iotests/051.s390.out
>>
>> I didn't even know we had a Makefile in tests/qemu-iotests. I don't
>> think it is invoked automatically, and if it was, it should not modify
>> the source tree. For instance, I have a separate build directory so my
>> source tree remains clean.
>>
>> I don't think expecting the user to invoke the Makefile manually is a
>> good idea either. When I said "copy" I literally meant adding a copy of
>> 051.s390.out with this patch (even though it is ugly, I'd still consider
>> it better).
> 
> Adding a copy of 051.s390.out is not perfect, but acceptable.
> 
>>
>> Anyway, we can circumvent the whole issue anyway. While 051 does test
>> some devices explicitly which are only available on the PC platform, the
>> thing in question which would require a s390-specific output, too, is
>> the final part of the test ("=== Snapshot mode ===").
>>
>> You can just drop the "case" introduced by this patch and set device_id
>> to whatever you want (e.g. "drive0" or "none0"). Then, you replace every
>> "-drive file..." by "-drive if=none,id=$device_id,file=..." and you're
>> done (obviously, the reference output needs to be adjusted for the
>> changed drive ID).
>>
>> Then, you don't need an 051.s390.out at all, and can just make that the
>> common output (051.out).
> 
> I got your idea. we can get the common output(051.out) for the final
> part of the test ("=== Snapshot mode ===") with this solution.
> 
> However, other parts of this test has different outputs as below,
> 1. s390x has no output for some test commands for "=== No medium ===",
> "=== Read-only ===" and "=== Cache modes ===". for example, "line 204 -
> 209" has output for pc, but no output for s390x.
> 
>     202 case "$QEMU_DEFAULT_MACHINE" in
>     203     pc)
>     204         run_qemu -drive media=cdrom,cache=none
>     205         run_qemu -drive media=cdrom,cache=directsync
>     206         run_qemu -drive media=cdrom,cache=writeback
>     207         run_qemu -drive media=cdrom,cache=writethrough
>     208         run_qemu -drive media=cdrom,cache=unsafe
>     209         run_qemu -drive media=cdrom,cache=invalid_value
>     210         ;;
>     211      *)
>     212         ;;
>     213 esac

Yes, indeed. However, it has this output only for pc, but it will not
have any output for any other platform type. Therefore, we don't need an
s390-specific reference output, but only one reference output for pc
(051.pc.out) and one for all the other platforms (051.out).

> 2. s390x has "Warning: Orphaned drive without device:" for
> run_qemu -drive file="$TEST_IMG",if=scsi,readonly=on
> run_qemu -drive if=scsi
> 
> Please refer
> http://lists.nongnu.org/archive/html/qemu-devel/2015-11/msg04836.html.

Yes, and that's exactly the reason why I advocated filtering out that
line because we don't know whether it is visible on any platform but s390.

(e.g. see the _filter_orphan added in
http://lists.nongnu.org/archive/html/qemu-devel/2015-05/msg05992.html)

Even if we decide against adding such a filter I think we should put the
s390 output into the common 051.out file anyway. If someone tries to run
the tests on a non-s390 and non-pc platform and does not get this
warning, thus failing the test, we can still talk about adding that filter.

> I plan to change the final part of the test("=== Snapshot mode ===")
> with your solution, and adding a copy of 051.s390.out. If this is fine
> to you, I plan to send a new version of patch for review asap, since
> Christmas is coming :-)

I'm fine with patches after christmas, too. ;-)

If you change the final part of the test and put the output into 051.out
instead of 051.s390.out (and don't create any copy for s390-virtio),
that should be fine.

Max

signature.asc
Description: OpenPGP digital signature