From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56137) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yn0nk-00083q-Fn for qemu-devel@nongnu.org; Tue, 28 Apr 2015 04:23:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yn0nd-0000VX-Gy for qemu-devel@nongnu.org; Tue, 28 Apr 2015 04:23:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38343) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yn0nd-0000Uv-AK for qemu-devel@nongnu.org; Tue, 28 Apr 2015 04:23:25 -0400 Date: Tue, 28 Apr 2015 10:23:22 +0200 From: Kevin Wolf Message-ID: <20150428082322.GA4378@noname.redhat.com> References: <1429756938-17186-1-git-send-email-chenxg@linux.vnet.ibm.com> <1429756938-17186-8-git-send-email-chenxg@linux.vnet.ibm.com> <553926D3.9010904@redhat.com> <553DE1FC.6040000@linux.vnet.ibm.com> <553E1C56.8020901@redhat.com> <20150427113459.GE4046@noname.str.redhat.com> <553EF7A7.7000102@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <553EF7A7.7000102@linux.vnet.ibm.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH RFC v7 7/7] qemu-iotests-s390x-fix-test-130 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tu bo Cc: armbru@redhat.com, mimu@linux.vnet.ibm.com, qemu-devel@nongnu.org, Max Reitz Am 28.04.2015 um 04:59 hat tu bo geschrieben: > Hi Kevin: >=20 > On 04/27/2015 07:34 PM, Kevin Wolf wrote: >=20 > Am 27.04.2015 um 13:24 hat Max Reitz geschrieben: >=20 > On 27.04.2015 09:15, tu bo wrote: >=20 > Hi Max: >=20 > On 04/24/2015 01:07 AM, Max Reitz wrote: >=20 > Well, that's a peculiar commit title. :-) >=20 > I guess it's supposed to be "qemu-iotests: s390x: fix t= est 130"? >=20 > You're right. I will change it in the next version :-) >=20 >=20 > On 23.04.2015 04:42, Xiao Guang Chen wrote: >=20 > From: Bo Tu >=20 > The tests for device type "ide_cd" should only be t= ested for the pc > platform. > The default device id of hard disk on the s390 plat= form differs to > that > of the x86 platform. A new variable device_id is de= fined and > "virtio0" > set for the s390 platform. A x86 platform specific = output file is > also > needed. >=20 > Signed-off-by: Bo Tu > --- > =A0 tests/qemu-iotests/130=A0=A0=A0=A0=A0=A0=A0 | 1= 3 +++++++++++-- > =A0 tests/qemu-iotests/130.out=A0=A0=A0 |=A0 4 ++-- > =A0 tests/qemu-iotests/130.pc.out | 43 > +++++++++++++++++++++++++++++++++++++++++++ > =A0 3 files changed, 56 insertions(+), 4 deletions(= -) > =A0 create mode 100644 tests/qemu-iotests/130.pc.ou= t >=20 > diff --git a/tests/qemu-iotests/130 b/tests/qemu-io= tests/130 > index bc26247..de40c7b 100755 > --- a/tests/qemu-iotests/130 > +++ b/tests/qemu-iotests/130 > @@ -58,9 +58,18 @@ echo "=3D=3D=3D HMP commit =3D=3D= =3D" > =A0 echo > =A0 # bdrv_make_empty() involves a header update fo= r qcow2 > =A0 +case "$QEMU_DEFAULT_MACHINE" in > +=A0=A0=A0 pc) > +=A0=A0=A0=A0=A0=A0=A0 device_id=3D"ide0-hd0" > +=A0=A0=A0=A0=A0=A0=A0 ;; > +=A0=A0=A0 s390) > +=A0=A0=A0=A0=A0=A0=A0 device_id=3D"virtio0" > +=A0=A0=A0=A0=A0=A0=A0 ;; >=20 >=20 > 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 machi= ne 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 s= omething we can > avoid (like defaulting to virtio0 for any non-pc platfo= rm). >=20 > Anyway, because I seem to remember I accepted it before= : >=20 > With the commit title fixed: >=20 > Reviewed-by: Max Reitz >=20 > I guess you discussed with Xiao Guang Chen and accepted it = in "[PATCH RFC > v5 6/7] qemu-iotests s390x fix test-051",=A0 because test 1= 30 and 051 are > using the same fix solution, and test 051 was fixed in v5. = Seeing section > of v5 in cover letter as below: >=20 >=20 > Indeed we discussed it. Just for clarification, I disliked havi= ng only cases > for "pc" and "s390" -- there are other platforms, too, which wi= ll 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 pr= obably work for > most without having to do further work here. >=20 > 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 thes= e test cases try > to use IDE devices which will not be available there). So the s= eries will not > break them because they didn't work before either. >=20 > Bottom line: I'm fine with this solution as it is. >=20 > 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? >=20 > Please forgive me that I'm very sure about the meaning of "default ID"=A0= you > mentioned. Maybe you mean "default device ID"? If I'm wrong, please cor= rect me > :-) >=20 > The default device id of hard disk on the s390 platform differs to the = device > id on the x86 platform,=A0 so we need to use different device id for di= fferent > platform. For instance,=A0 using "virtio0" for s390x, and using "ide0-h= d0" for > x86 as below: > +_send_qemu_cmd $QEMU_HANDLE "commit "=A0 "virtio0" "(qemu)" > +_send_qemu_cmd $QEMU_HANDLE "commit "=A0 "ide0-hd0" "(qemu)" Any why not use this? qemu -drive id=3Dtestdisk,... (qemu) commit testdisk That would be the same on all platforms. Kevin