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

[tu bo <tubo@linux.vnet.ibm.com>]

Hi Max:

On 12/02/2015 11:48 PM, Max Reitz wrote:
> 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 <tubo@linux.vnet.ibm.com>
>>>>
>>>> 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 <mreitz@redhat.com>
>>>
>>> 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 <silbe@linux.vnet.ibm.com>
>>>> Signed-off-by: Bo Tu <tubo@linux.vnet.ibm.com>
>>>> ---
>>>>    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 prefer to keep the warning in 051.out.

>
>> 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.

Sure, I'll put the output into 051.out for any non-pc platform, and 
remove 051.s390.out(and rollback the changes in Makefile).  thanks

>
> Max
>