qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH] iotests: Check for enabled drivers before testing them
@ 2019-08-19  7:53 Thomas Huth
  2019-08-19  8:22 ` no-reply
  2019-08-20 15:01 ` Max Reitz
  0 siblings, 2 replies; 7+ messages in thread
From: Thomas Huth @ 2019-08-19  7:53 UTC (permalink / raw)
  To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Max Reitz

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.

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
 
 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
 
 do_run_qemu()
 {
diff --git a/tests/qemu-iotests/099 b/tests/qemu-iotests/099
index ae02f27afe..c3cf66798a 100755
--- a/tests/qemu-iotests/099
+++ b/tests/qemu-iotests/099
@@ -42,6 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 _supported_fmt qcow qcow2 qed vdi vhdx vmdk vpc
 _supported_proto file
 _supported_os Linux
+_require_drivers blkdebug blkverify
 _unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat" \
     "subformat=twoGbMaxExtentSparse"
 
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
 
 do_run_qemu()
 {
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:')
+    for driver
+    do
+        if ! echo "$available" | grep -q "$driver"; then
+            _notrun "$driver not available"
+        fi
+    done
+}
+
 # make sure this script returns success
 true
-- 
2.18.1



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH] iotests: Check for enabled drivers before testing them
  2019-08-19  7:53 [Qemu-devel] [PATCH] iotests: Check for enabled drivers before testing them Thomas Huth
@ 2019-08-19  8:22 ` no-reply
  2019-08-20 15:01 ` Max Reitz
  1 sibling, 0 replies; 7+ messages in thread
From: no-reply @ 2019-08-19  8:22 UTC (permalink / raw)
  To: thuth; +Cc: kwolf, qemu-devel, qemu-block, mreitz

Patchew URL: https://patchew.org/QEMU/20190819075348.4078-1-thuth@redhat.com/



Hi,

This series failed build test on s390x host. Please find the details below.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
# Testing script will be invoked under the git checkout with
# HEAD pointing to a commit that has the patches applied on top of "base"
# branch
set -e

echo
echo "=== ENV ==="
env

echo
echo "=== PACKAGES ==="
rpm -qa

echo
echo "=== UNAME ==="
uname -a

CC=$HOME/bin/cc
INSTALL=$PWD/install
BUILD=$PWD/build
mkdir -p $BUILD $INSTALL
SRC=$PWD
cd $BUILD
$SRC/configure --cc=$CC --prefix=$INSTALL
make -j4
# XXX: we need reliable clean up
# make check -j4 V=1
make install
=== TEST SCRIPT END ===

  CC      mips-softmmu/trace/control-target.o
  CC      mips-softmmu/trace/generated-helpers.o
  LINK    mips-softmmu/qemu-system-mips
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:209: qemu-system-mipsel] Error 1
make: *** [Makefile:472: mipsel-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
---
  CC      hppa-softmmu/hw/block/vhost-user-blk.o
  CC      microblaze-softmmu/balloon.o
  CC      hppa-softmmu/hw/block/dataplane/virtio-blk.o
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:209: qemu-system-mips] Error 1
make: *** [Makefile:472: mips-softmmu/all] Error 2
  CC      hppa-softmmu/hw/char/virtio-serial-bus.o
---
  CC      microblaze-softmmu/trace/generated-helpers.o
  LINK    microblaze-softmmu/qemu-system-microblaze
  LINK    hppa-softmmu/qemu-system-hppa
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:209: qemu-system-microblaze] Error 1
make: *** [Makefile:472: microblaze-softmmu/all] Error 2
collect2: error: ld returned 1 exit status
make[1]: *** [Makefile:209: qemu-system-hppa] Error 1
make: *** [Makefile:472: hppa-softmmu/all] Error 2
=== OUTPUT END ===


The full log is available at
http://patchew.org/logs/20190819075348.4078-1-thuth@redhat.com/testing.s390x/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH] iotests: Check for enabled drivers before testing them
  2019-08-19  7:53 [Qemu-devel] [PATCH] iotests: Check for enabled drivers before testing them Thomas Huth
  2019-08-19  8:22 ` no-reply
@ 2019-08-20 15:01 ` Max Reitz
  2019-08-20 16:01   ` Thomas Huth
  1 sibling, 1 reply; 7+ messages in thread
From: Max Reitz @ 2019-08-20 15:01 UTC (permalink / raw)
  To: Thomas Huth, qemu-block; +Cc: Kevin Wolf, qemu-devel


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

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

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



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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH] iotests: Check for enabled drivers before testing them
  2019-08-20 15:01 ` Max Reitz
@ 2019-08-20 16:01   ` Thomas Huth
  2019-08-20 18:48     ` Max Reitz
  0 siblings, 1 reply; 7+ messages in thread
From: Thomas Huth @ 2019-08-20 16:01 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH] iotests: Check for enabled drivers before testing them
  2019-08-20 16:01   ` Thomas Huth
@ 2019-08-20 18:48     ` Max Reitz
  2019-08-20 19:19       ` Thomas Huth
  0 siblings, 1 reply; 7+ messages in thread
From: Max Reitz @ 2019-08-20 18:48 UTC (permalink / raw)
  To: Thomas Huth, qemu-block; +Cc: Kevin Wolf, qemu-devel


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH] iotests: Check for enabled drivers before testing them
  2019-08-20 18:48     ` Max Reitz
@ 2019-08-20 19:19       ` Thomas Huth
  2019-08-20 19:23         ` Max Reitz
  0 siblings, 1 reply; 7+ messages in thread
From: Thomas Huth @ 2019-08-20 19:19 UTC (permalink / raw)
  To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel


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

On 8/20/19 8:48 PM, Max Reitz wrote:
> On 20.08.19 18:01, Thomas Huth wrote:
[...]
>> 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

Well, let's put it this way: The less changes we have to carry along
downstream (and thus review each time we rebase the downstream tree),
the more time we have to work on upstream.

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

Ok, fine for me, too, if we also declare "null-co" as optional for the
iotests - let's make sure that the tests in the "auto" group also work
without them.

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

IIRC I tried to run all qcow2 tests when I disabled null-co and saw lots
of failures ... but anyway, let's just focus on the "auto" tests right
now, that should be doable.

 Thomas


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Qemu-devel] [PATCH] iotests: Check for enabled drivers before testing them
  2019-08-20 19:19       ` Thomas Huth
@ 2019-08-20 19:23         ` Max Reitz
  0 siblings, 0 replies; 7+ messages in thread
From: Max Reitz @ 2019-08-20 19:23 UTC (permalink / raw)
  To: Thomas Huth, qemu-block; +Cc: Kevin Wolf, qemu-devel


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

On 20.08.19 21:19, Thomas Huth wrote:
> On 8/20/19 8:48 PM, Max Reitz wrote:
>> On 20.08.19 18:01, Thomas Huth wrote:
> [...]
>>> 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
> 
> Well, let's put it this way: The less changes we have to carry along
> downstream (and thus review each time we rebase the downstream tree),
> the more time we have to work on upstream.

As I said, I’m guilty myself.

>> 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.
> 
> Ok, fine for me, too, if we also declare "null-co" as optional for the
> iotests - let's make sure that the tests in the "auto" group also work
> without them.

Well, should we or not?  You said there are other tests (outside of the
iotests) that break without null-co.  If so, I don’t think there’s any
point in making it optional here.

>>>> 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).
> 
> IIRC I tried to run all qcow2 tests when I disabled null-co and saw lots
> of failures ... but anyway, let's just focus on the "auto" tests right
> now, that should be doable.

OK, I didn’t bother running all. :-)

Max


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-08-20 19:28 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-19  7:53 [Qemu-devel] [PATCH] iotests: Check for enabled drivers before testing them Thomas Huth
2019-08-19  8:22 ` no-reply
2019-08-20 15:01 ` Max Reitz
2019-08-20 16:01   ` Thomas Huth
2019-08-20 18:48     ` Max Reitz
2019-08-20 19:19       ` Thomas Huth
2019-08-20 19:23         ` Max Reitz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).