Re: [PATCH v8 07/17] qemu-iotests, qtest: rewrite test 067 as a qtest

[Kevin Wolf <kwolf@redhat.com>]

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 <pbonzini@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 <kwolf@redhat.com>