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

[Kevin Wolf]

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 <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.
> 
>                     Signed-off-by: Bo Tu <address@hidden>
>                     ---
>                       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 <address@hidden>
> 
>             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

That would be the same on all platforms.

Kevin