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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3520 bytes --]

On 27.04.2015 09:15, tu bo wrote:
> Hi Max:
>
> On 04/24/2015 01:07 AM, Max Reitz wrote:
>> Well, that's a peculiar commit title. :-)
>>
>> I guess it's supposed to be "qemu-iotests: s390x: fix test 130"?
> You're right. I will change it in the next version :-)
>>
>> On 23.04.2015 04:42, Xiao Guang Chen 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.
>>>
>>> Signed-off-by: Bo Tu <tubo@linux.vnet.ibm.com>
>>> ---
>>>   tests/qemu-iotests/130        | 13 +++++++++++--
>>>   tests/qemu-iotests/130.out    |  4 ++--
>>>   tests/qemu-iotests/130.pc.out | 43 
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 56 insertions(+), 4 deletions(-)
>>>   create mode 100644 tests/qemu-iotests/130.pc.out
>>>
>>> diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130
>>> index bc26247..de40c7b 100755
>>> --- a/tests/qemu-iotests/130
>>> +++ b/tests/qemu-iotests/130
>>> @@ -58,9 +58,18 @@ echo "=== HMP commit ==="
>>>   echo
>>>   # bdrv_make_empty() involves a header update for qcow2
>>>   +case "$QEMU_DEFAULT_MACHINE" in
>>> +    pc)
>>> +        device_id="ide0-hd0"
>>> +        ;;
>>> +    s390)
>>> +        device_id="virtio0"
>>> +        ;;
>>
>> I think I mentioned before that I don't really like not taking the 
>> fact into account that there are other machine types, too. I'm still 
>> accepting it based on the fact that I think those machine types won't 
>> pass the tests right now anyway, so not caring for them in these case 
>> blocks won't break any tests, but it still feels like something we 
>> can avoid (like defaulting to virtio0 for any non-pc platform).
>>
>> Anyway, because I seem to remember I accepted it before:
>>
>> With the commit title fixed:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
> I guess you discussed with Xiao Guang Chen and accepted it in "[PATCH 
> RFC v5 6/7] qemu-iotests s390x fix test-051",  because test 130 and 
> 051 are using the same fix solution, and test 051 was fixed in v5. 
> Seeing section of v5 in cover letter as below:

Indeed we discussed it. Just for clarification, I disliked having only 
cases for "pc" and "s390" -- there are other platforms, too, which will 
simply break by not including them in these case statements. We could 
try to avoid this by defaulting to virtio0 for every non-pc platform, 
and it will probably work for most without having to do further work here.

However, I did accept it because all those non-PC (and non-s390) 
platforms won't pass the tests before this patch set either (because 
these test cases try to use IDE devices which will not be available 
there). So the series will not break them because they didn't work 
before either.

Bottom line: I'm fine with this solution as it is.

Max

> /v5://
> //1. Add a pc specific output file for test 051.//
> //2. Add a filter to test case 051 to filter s390 specific warnings.//
> //3. Check whether the machine type is pc or not rather than check 
> whether the machine type //
> //is s390.//
> //4. When using a machine specific reference file if the default 
> machine has an alias then//
> //use the alias as the output file name otherwise use the default 
> machine name as the output//
> //file name/

[-- Attachment #2: Type: text/html, Size: 5651 bytes --]