From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54100) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a49eR-0007FV-1C for qemu-devel@nongnu.org; Wed, 02 Dec 2015 10:49:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a49eN-0002QO-JU for qemu-devel@nongnu.org; Wed, 02 Dec 2015 10:49:02 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58160) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a49eN-0002Q3-CZ for qemu-devel@nongnu.org; Wed, 02 Dec 2015 10:48:59 -0500 References: <1448531594-27192-1-git-send-email-tubo@linux.vnet.ibm.com> <1448531594-27192-3-git-send-email-tubo@linux.vnet.ibm.com> <565CA5C0.1000800@redhat.com> <565D4DC0.7070402@linux.vnet.ibm.com> From: Max Reitz Message-ID: <565F12E6.2090803@redhat.com> Date: Wed, 2 Dec 2015 16:48:54 +0100 MIME-Version: 1.0 In-Reply-To: <565D4DC0.7070402@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="qsuPoRngf5765UVGU74X679IDAexqo0EN" Subject: Re: [Qemu-devel] [PATCH v3 2/3] qemu-iotests: s390x: fix test 051 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tu bo , qemu-devel@nongnu.org Cc: kwolf@redhat.com, silbe@linux.vnet.ibm.com, armbru@redhat.com, mimu@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --qsuPoRngf5765UVGU74X679IDAexqo0EN Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01.12.2015 08:35, tu bo wrote: > Hi Max: >=20 > =E5=9C=A8 2015/12/1 3:38, Max Reitz =E5=86=99=E9=81=93: >> On 26.11.2015 10:53, Bo Tu wrote: >>> From: Bo Tu >>> >>> 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 th= at >>> 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 als= o >>> needed. >>> Warning message expected for s390x when drive without device. >>> >>> Reviewed-by: Max Reitz >> >> I can't remember having given this for this patch. ;-) >=20 > It's my mistake, please forgive me for it. No problem. >>> Reviewed-by: Sascha Silbe >>> Signed-off-by: Bo Tu >>> --- >>> 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 o= f >> 051.s390.out with this patch (even though it is ugly, I'd still consid= er >> it better). >=20 > Adding a copy of 051.s390.out is not perfect, but acceptable. >=20 >> >> Anyway, we can circumvent the whole issue anyway. While 051 does test >> some devices explicitly which are only available on the PC platform, t= he >> thing in question which would require a s390-specific output, too, is >> the final part of the test ("=3D=3D=3D Snapshot mode =3D=3D=3D"). >> >> You can just drop the "case" introduced by this patch and set device_i= d >> to whatever you want (e.g. "drive0" or "none0"). Then, you replace eve= ry >> "-drive file..." by "-drive if=3Dnone,id=3D$device_id,file=3D..." 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 th= e >> common output (051.out). >=20 > I got your idea. we can get the common output(051.out) for the final > part of the test ("=3D=3D=3D Snapshot mode =3D=3D=3D") with this soluti= on. >=20 > However, other parts of this test has different outputs as below, > 1. s390x has no output for some test commands for "=3D=3D=3D No medium = =3D=3D=3D", > "=3D=3D=3D Read-only =3D=3D=3D" and "=3D=3D=3D Cache modes =3D=3D=3D". = for example, "line 204 - > 209" has output for pc, but no output for s390x. >=20 > 202 case "$QEMU_DEFAULT_MACHINE" in > 203 pc) > 204 run_qemu -drive media=3Dcdrom,cache=3Dnone > 205 run_qemu -drive media=3Dcdrom,cache=3Ddirectsync > 206 run_qemu -drive media=3Dcdrom,cache=3Dwriteback > 207 run_qemu -drive media=3Dcdrom,cache=3Dwritethrough > 208 run_qemu -drive media=3Dcdrom,cache=3Dunsafe > 209 run_qemu -drive media=3Dcdrom,cache=3Dinvalid_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=3D"$TEST_IMG",if=3Dscsi,readonly=3Don > run_qemu -drive if=3Dscsi >=20 > 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= =2E (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 filte= r. > I plan to change the final part of the test("=3D=3D=3D Snapshot mode =3D= =3D=3D") > 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 --qsuPoRngf5765UVGU74X679IDAexqo0EN Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWXxLmAAoJEDuxQgLoOKyt6VsH/ii0IU+/IxsfROuZpSKngDyY 5IHXw5RyRDEy+M3AjE7ySsOywIosWmlkeZ9B+49ZXZCyW7m5lAm+LfkdHTcYDtgA KWRG7y0gopqbuSmlr8fbH0xqiSvT5wdqmKtVamshjp1Gsce2X5ojAWLAEr7dXCVN RvtL0V6vHqBmNmqRkqO63ZqaX699upUbCCccq1/e2UWJPb45xwNBP/aXtz7Woo2L VpwnnxldOlIF2mGpzYl67ggMrncXI0OGOAu7Ahsizz7UuIgRk8J9lV89SOgqUpO4 Lg6iAihuK+4xs7C7bJ3vnAAQq+dnWyifLHKkmQ7qiDPp3hZsYlrjmWOCsl8XspE= =EQWx -----END PGP SIGNATURE----- --qsuPoRngf5765UVGU74X679IDAexqo0EN--