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 <t...@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 <mre...@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 <si...@linux.vnet.ibm.com>
>>> Signed-off-by: Bo Tu <t...@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 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
