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

[Max Reitz]

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.

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?

> Signed-off-by: Thomas Huth <address@hidden>
> ---
>  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.

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

(Also, this test requires the raw driver.)

>  do_run_qemu()
>  {

[...]

> 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

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

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

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

Max

> +            _notrun "$driver not available"
> +        fi
> +    done
> +}
> +
>  # make sure this script returns success
>  true
>

signature.asc
Description: OpenPGP digital signature