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

[Paolo Bonzini]

On 09/10/20 11:48, Kevin Wolf wrote:
> 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.

Agreed.

> 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.)

Ok, I will.

>> +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?

query-block doesn't really test anything here because there's no drive
at all.  Reset also doesn't do much that is interesting for scsi-cd but
I can add it back.

Paolo

> 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>
>