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 <th...@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
signature.asc
Description: OpenPGP digital signature