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

[Thomas Huth <thuth@redhat.com>]

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

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.

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

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

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

> (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 ;-)

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

>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 5502c3da2f..7d4e68846f 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -520,5 +520,18 @@ _require_command()
>>      [ -x "$c" ] || _notrun "$1 utility required, skipped this test"
>>  }
>>  
>> +# Check that a set of drivers has been whitelisted in the QEMU binary
>> +#
>> +_require_drivers()
>> +{
>> +    available=$($QEMU -drive format=help | grep 'Supported formats:')
> Seems a bit shortcut-y to not remove the “Supported formats:” prefix,
> but I don’t suppose we’ll ever have block drivers with either name...

I can remove it, just to be sure.

>> +    for driver
>> +    do
>> +        if ! echo "$available" | grep -q "$driver"; then
> 
> 162 greps like this:
> 
>> grep '^Supported formats:.* ssh\( \|$\)'
> 
> Maybe the same should be done here, i.e. grep -q " $driver\( \|\$\)"?  I
> can well imagine that something like “ssh” might appear as a substring
> in some other driver.

Fair, I'll add the spaces.

> (Speaking of which, why not change 162 to using this new function?  Yes,
> it isn’t in auto, but still...)

Yes, I can add that.

 Thomas


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