| Message ID | 1455023713-104799-4-git-send-email-silbe@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09.02.2016 14:15, Sascha Silbe wrote: > IDE is only implemented by very few architectures (mostly PC). Use > virtio-scsi instead so the test works on all architectures that > support virtio. In particular, this fixes qemu-iotests on s390x. > > Fixes: 16dee418 ("iotests: Add test for eject under NBD server") > Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com> > --- > tests/qemu-iotests/140 | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140 > index f78c317..0c448e6 100755 > --- a/tests/qemu-iotests/140 > +++ b/tests/qemu-iotests/140 > @@ -49,8 +49,8 @@ _make_test_img 64k > $QEMU_IO -c 'write -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io > > keep_stderr=y \ > -_launch_qemu -drive if=ide,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \ > - 2> >(_filter_nbd) > +_launch_qemu -drive if=none,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \ > + -device virtio-scsi -device scsi-cd,drive=drv 2> >(_filter_nbd) Why not just omit the device (and the media=cdrom along with it, keeping if=none)? This will change the reference output because there is no longer any tray to be moved, but this will still test what it's supposed to. (This may sound hypocritical coming from me, because I wrote this test so I could have just done so in the first place; I guess I just didn't realize that 'eject' works on device-less drives, too.) Max > _send_qemu_cmd $QEMU_HANDLE \ > "{ 'execute': 'qmp_capabilities' }" \ >
Dear Max, Max Reitz <mreitz@redhat.com> writes: [tests/qemu-iotests/140] >> -_launch_qemu -drive if=ide,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \ >> - 2> >(_filter_nbd) >> +_launch_qemu -drive if=none,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \ >> + -device virtio-scsi -device scsi-cd,drive=drv 2> >(_filter_nbd) > > Why not just omit the device (and the media=cdrom along with it, keeping > if=none)? This will change the reference output because there is no > longer any tray to be moved, but this will still test what it's supposed to. > > (This may sound hypocritical coming from me, because I wrote this test > so I could have just done so in the first place; I guess I just didn't > realize that 'eject' works on device-less drives, too.) Is this supposed to work? I.e. can we rely on it? If so, that would certainly be the easier route for this particular test. Test coverage should be unaffected as 139 already tests ejection (using virtio, unlike 118 which is PC-only). The aliases patch has a value of its own, but that's a separate matter. Sascha
On 10.02.2016 20:26, Sascha Silbe wrote: > Dear Max, > > Max Reitz <mreitz@redhat.com> writes: > > [tests/qemu-iotests/140] >>> -_launch_qemu -drive if=ide,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \ >>> - 2> >(_filter_nbd) >>> +_launch_qemu -drive if=none,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \ >>> + -device virtio-scsi -device scsi-cd,drive=drv 2> >(_filter_nbd) >> >> Why not just omit the device (and the media=cdrom along with it, keeping >> if=none)? This will change the reference output because there is no >> longer any tray to be moved, but this will still test what it's supposed to. >> >> (This may sound hypocritical coming from me, because I wrote this test >> so I could have just done so in the first place; I guess I just didn't >> realize that 'eject' works on device-less drives, too.) > > Is this supposed to work? I.e. can we rely on it? Let's say I would rely on it. :-) (which is why I proposed it) The test checks that ejecting a BlockDriverState tree from a BlockBackend (that is, a medium from a drive) works even if that BlockBackend is exposed via an NBD server. It doesn't really matter whether the drive has a device or not, the main thing is that the NBD server notices that the medium is ejected and automatically stops offering the drive. So I would think that we are supposed to be able to rely on it; if we cannot, something else is probably broken. > If so, that would > certainly be the easier route for this particular test. Test coverage > should be unaffected as 139 already tests ejection (using virtio, unlike > 118 which is PC-only). > > The aliases patch has a value of its own, but that's a separate > matter. Yes, certainly. Max
diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140 index f78c317..0c448e6 100755 --- a/tests/qemu-iotests/140 +++ b/tests/qemu-iotests/140 @@ -49,8 +49,8 @@ _make_test_img 64k $QEMU_IO -c 'write -P 42 0 64k' "$TEST_IMG" | _filter_qemu_io keep_stderr=y \ -_launch_qemu -drive if=ide,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \ - 2> >(_filter_nbd) +_launch_qemu -drive if=none,media=cdrom,id=drv,file="$TEST_IMG",format=$IMGFMT \ + -device virtio-scsi -device scsi-cd,drive=drv 2> >(_filter_nbd) _send_qemu_cmd $QEMU_HANDLE \ "{ 'execute': 'qmp_capabilities' }" \
IDE is only implemented by very few architectures (mostly PC). Use virtio-scsi instead so the test works on all architectures that support virtio. In particular, this fixes qemu-iotests on s390x. Fixes: 16dee418 ("iotests: Add test for eject under NBD server") Signed-off-by: Sascha Silbe <silbe@linux.vnet.ibm.com> --- tests/qemu-iotests/140 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)