Re: [Qemu-devel] [PATCH] iotests: Check for enabled drivers before testing them

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6318 bytes --]

On 20.08.19 18:01, Thomas Huth wrote:
> On 8/20/19 5:01 PM, Max Reitz wrote:
>> On 19.08.19 09:53, Thomas Huth wrote:
>>> It is possible to enable only a subset of the block drivers with the
>>> "--block-drv-rw-whitelist" option of the "configure" script. All other
>>> drivers are marked as unusable (or only included as read-only with the
>>> "--block-drv-ro-whitelist" option). If an iotest is now using such a
>>> disabled block driver, it is failing - which is bad, since at least the
>>> tests in the "auto" group should be able to deal with this situation.
>>> Thus let's introduce a "_require_drivers" function that can be used by
>>> the shell tests to check for the availability of certain drivers first,
>>> and marks the test as "not run" if one of the drivers is missing.
>>
>> Well, the reasoning for generally not making blkdebug/blkverify explicit
>> requirements was that you should just have both enabled when running
>> iotests.
> 
> Well, we disable blkverify in our downstream RHEL version of QEMU - so
> it would be great if the iotests could at least adapt to that missing
> driver.

I would like to say that RHEL is not a gold standard, but then I have
this series of selfish patches that specifically addresses RHEL
whitelisting issues.

It feels a bit weird to me to say “blkverify is not essential, because
RHEL disables it, but null-co is” – even though there is no reason why
anyone would need null-co except for testing either.

>> Of course, that no longer works as an argument now that we
>> unconditionally run some iotests in make check.
>>
>> But still, the question is how strict you want to be.  If blkdebug
>> cannot be assumed to be present, what about null-co?  What about raw?
> 
> I tried to disable everything beside qcow2 - but that causes so many
> things to fail that it hardly makes sense to try to get that working.

Hm, really?  I just whitelisted qcow2 and file and running the auto
group worked rather well (except for the failing tests you address here,
and the two others I mentioned).

> I think we can assume that at least null-co, qcow2 and raw are enabled.
> (If anybody still wants to try to run "make check" with one of these
> drivers disabled, I think we should rather add a superior check to
> tests/check-block.sh or tests/qemu-iotests/check instead and skip the
> iotests completely in that case).

I’m OK either way: We can add a global check, or we just make a decision
on what users definitely have to have enabled or the qemu build would be
a bit boring.

Assuming file, qcow2, and raw to be enabled seems reasonable.  I’m not
so sure about null-co.

(I mean, I personally don’t really care.  I never added such checks
myself, even a bit purposefully so, because it was my opinion that you
should probably have all block drivers whitelisted before running the
iotests.  But then came CI and make check-block integration...)

((Also, you’ll notice that I was really inconsequential about adding
null-co checks in my “selfish patches” series, precisely because I
assumed everyone would have whitelisted null-co.  So I’m indeed OK with
just making it an implicit prerequisite.))

>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  tests/qemu-iotests/071       |  1 +
>>>  tests/qemu-iotests/081       |  1 +
>>>  tests/qemu-iotests/099       |  1 +
>>>  tests/qemu-iotests/184       |  1 +
>>>  tests/qemu-iotests/common.rc | 13 +++++++++++++
>>>  5 files changed, 17 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
>>> index 1cca9233d0..fab526666b 100755
>>> --- a/tests/qemu-iotests/071
>>> +++ b/tests/qemu-iotests/071
>>> @@ -38,6 +38,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>>  
>>>  _supported_fmt qcow2
>>>  _supported_proto file
>>> +_require_drivers blkdebug blkverify
>>
>> Because this test also requires the raw driver.
> 
> The test also works for me when I configured QEMU with:
> 
>  --block-drv-rw-whitelist="qcow2 file null-co blkdebug blkverify"
> 
> i.e. the raw driver should be disabled in that case?

Ah, it’s just used by qemu-io, which of course ignores whitelisting.

>>>  
>>>  do_run_qemu()
>>>  {
>>> diff --git a/tests/qemu-iotests/081 b/tests/qemu-iotests/081
>>> index c418bab093..1695781bc0 100755
>>> --- a/tests/qemu-iotests/081
>>> +++ b/tests/qemu-iotests/081
>>> @@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>>>  _supported_fmt raw
>>>  _supported_proto file
>>>  _supported_os Linux
>>> +_require_drivers quorum
>>
>> This test has already a check whether quorum is supported, that should
>> be removed now.
> 
> Hmm, true ... apparently that test was not working for my case ... could
> it be that qemu-img ignores the whitelist and simply says that all
> drivers are supported?

Ah, yeah.  The whitelist is relevant only for the system emulator.

I forgot why we even had the existing check, then.  Maybe just a mistake
to use qemu-img.

>> (Also, this test requires the raw driver.)
> 
> Agreed, this test indeed does not work without 'raw'. But it is already
> marked with "_supported_fmt raw", so you can indeed only run it with
> "raw". And running a raw-only test with a qemu binary where raw is
> disabled could be considered as user error, I guess ;-)

Oops, didn’t even notice somehow. O:-)

>>> diff --git a/tests/qemu-iotests/184 b/tests/qemu-iotests/184
>>> index cb0c181228..33dd8d2a4f 100755
>>> --- a/tests/qemu-iotests/184
>>> +++ b/tests/qemu-iotests/184
>>> @@ -33,6 +33,7 @@ trap "exit \$status" 0 1 2 3 15
>>>  . ./common.filter
>>>  
>>>  _supported_os Linux
>>> +_require_drivers throttle
>>
>> This test also requires null-co.
>>
>>>  do_run_qemu()
>>>  {
>>
>> I found two more check-block tests that may or may not require use of
>> _require_drivers (depending on which drivers we deem absolutely essential):
>> - 120: Needs raw
>> - 186: Needs null-co
> 
> I think we really have to assume that null-co is available, otherwise
> too many things break... (also some qtests are using null-co).
> 
> But I could for sure add a check for raw in 120 if desired.

If we assume that null-co is there, raw is definitely going to be there.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]