Am 07.10.2020 um 13:56 hat Paolo Bonzini geschrieben:
> Test 067 from qemu-iotests is executing QMP commands to hotplug
> and hot-unplug disks, devices and blockdevs. Because the power
> of the text-based test harness is limited, it is actually limiting
> the checks that it does, for example by skipping DEVICE_DELETED
> events.
>
> tests/qtest already has a similar test, drive_del-test.c.
> We can merge them, and even reuse some of the existing code in
> drive_del-test.c, and improve the quality of the test by
> covering DEVICE_DELETED events. The only difference is that
> the new test will always use null-co:// for the medium
> rather than qcow2 or raw, but this should be irrelevant
> for what the test is covering. For example there are
> no "qemu-img check" runs in 067 that would check that
> the file is properly closed.
>
> The new tests requires PCI hot-plug support, so drive_del-test
> is moved from qemu-system-ppc to qemu-system-ppc64.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
As discussed on IRC, I'm not a big fan of moving QMP tests that don't
make use of the qtest protocol at all to C unit tests (nothing in
drive_del_test makes use of the qtest protocol, neither before nor after
this patch). It's generally harder to write this kind of tests in C than
in Python, and assertion based tests are harder to debug than reference
output based ones.
There is one argument why this should be a qtest, which is that qtests
are run for multitple guest architectures while iotests run only for the
first architecture we found. I'm not sure if it's a good argument, but I
can't completely dismiss it.
The commit message should mention this argument, though.
In the future, I think iotests should be extended to provide the
necessary infrastructure to run tests on several architectures, and then
this should be converted to a Python iotest.
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 9e4f7c0153..0d31fda111 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -88,7 +88,6 @@
> 064 rw quick
> 065 rw quick
> 066 rw auto quick
> -067 rw quick
> 068 rw quick
> 069 rw auto quick
> 070 rw quick
Please keep a comment that 067 shouldn't be reused, like we do for some
other cases. (It only causes merge conflicts for downstreams.)
> +static void test_empty_device_del(void)
> +{
> + QTestState *qts;
> +
> + /* device_del with no drive plugged. */
> + qts = qtest_initf("-device virtio-scsi-%s -device scsi-cd,id=dev0",
> + qvirtio_get_dev_type());
> +
> + device_del(qts, false);
> + qtest_quit(qts);
> +}
067 tested reset and query-block after this. Is the removal intentional?
Other than these, the conversion looks correct. I'm not convinced that
doing it is a step in the right direction, but with these two things
fixed, you can add:
Reviewed-by: Kevin Wolf <kw...@redhat.com>
