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

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

Hi Kevin:

On 04/28/2015 04:23 PM, Kevin Wolf wrote:
> Am 28.04.2015 um 04:59 hat tu bo geschrieben:
>> Hi Kevin:
>>
>> On 04/27/2015 07:34 PM, Kevin Wolf wrote:
>>
>>      Am 27.04.2015 um 13:24 hat Max Reitz geschrieben:
>>
>>          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.
>>
>>      Maybe I'm missing the obvious, but why don't you just specify an
>>      explicit ID instead of guessing the default ID that qemu will use
>>      depending on the platform?
>>
>> Please forgive me that I'm very sure about the meaning of "default ID"  you
>> mentioned. Maybe you mean "default device ID"? If I'm wrong, please correct me
>> :-)
>>
>> The default device id of hard disk on the s390 platform differs to the device
>> id on the x86 platform,  so we need to use different device id for different
>> platform. For instance,  using "virtio0" for s390x, and using "ide0-hd0" for
>> x86 as below:
>> +_send_qemu_cmd $QEMU_HANDLE "commit "  "virtio0" "(qemu)"
>> +_send_qemu_cmd $QEMU_HANDLE "commit "  "ide0-hd0" "(qemu)"
> Any why not use this?
>
> qemu -drive id=testdisk,...
> (qemu) commit testdisk
I tried and it works fine,  which is a more easier solution for test 
130.  I'll use your solution in patch v8.   thanks a lot
>
> That would be the same on all platforms.
>
> Kevin
>