| Message ID | 20180426161958.2872-1-rkagan@virtuozzo.com |
|---|---|
| Headers | show |
| Series | iotests: don't choke on disabled drivers | expand |
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180426161958.2872-1-rkagan@virtuozzo.com
Subject: [Qemu-devel] [PATCH 00/17] iotests: don't choke on disabled drivers
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
t [tag update] patchew/1524666934-8064-1-git-send-email-lidongchen@tencent.com -> patchew/1524666934-8064-1-git-send-email-lidongchen@tencent.com
* [new tag] patchew/20180426161958.2872-1-rkagan@virtuozzo.com -> patchew/20180426161958.2872-1-rkagan@virtuozzo.com
Switched to a new branch 'test'
073994ede0 iotest 184: notrun if null-co or throttle is disabled
0d6a103a2e iotest 147: notrun if nbd is disabled
3b6c88c400 iotest 139: skip testcases using disabled drivers
e6f12551db iotest 124: skip testcases using blkdebug if disabled
14a2cc874d iotest 099: notrun if blkdebug or blkverify is disabled
f0a42278cc iotest 093: notrun if null-co or null-aio is disabled
eda5b858c7 iotest 087: notrun if null-co is disabled
dd607cdf8a iotest 081: notrun if quorum is disabled
78c6ef361f iotest 071: notrun if blkdebug or blkverify is disabled
a04fb3d3e2 iotest 056: skip testcases using blkdebug if disabled
df221ccdea iotest 055: require blkdebug
710254744d iotest 055: skip unsupported backup target formats
c635939e50 iotest 030: require blkdebug
5338c53d0f iotest 030: skip quorum test setup/teardown too
ca24e281d8 iotests: ask qemu for supported formats
642020ff29 iotests: iotests.py: prevent deadlock in subprocess
0c0f7d7005 block: iterate_format with account of whitelisting
=== OUTPUT BEGIN ===
Checking PATCH 1/17: block: iterate_format with account of whitelisting...
Checking PATCH 2/17: iotests: iotests.py: prevent deadlock in subprocess...
ERROR: line over 90 characters
#29: FILE: tests/qemu-iotests/iotests.py:109:
+ sys.stderr.write('qemu-img received signal %i: %s\n' % (-subp.returncode, ' '.join(qemu_img_args + list(args))))
ERROR: line over 90 characters
#43: FILE: tests/qemu-iotests/iotests.py:119:
+ sys.stderr.write('qemu-io received signal %i: %s\n' % (-subp.returncode, ' '.join(args)))
total: 2 errors, 0 warnings, 28 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 3/17: iotests: ask qemu for supported formats...
ERROR: line over 90 characters
#58: FILE: tests/qemu-iotests/iotests.py:130:
+ sys.stderr.write('qemu received signal %i: %s\n' % (-subp.returncode, ' '.join(args)))
total: 1 errors, 0 warnings, 70 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 4/17: iotest 030: skip quorum test setup/teardown too...
Checking PATCH 5/17: iotest 030: require blkdebug...
Checking PATCH 6/17: iotest 055: skip unsupported backup target formats...
Checking PATCH 7/17: iotest 055: require blkdebug...
Checking PATCH 8/17: iotest 056: skip testcases using blkdebug if disabled...
Checking PATCH 9/17: iotest 071: notrun if blkdebug or blkverify is disabled...
Checking PATCH 10/17: iotest 081: notrun if quorum is disabled...
Checking PATCH 11/17: iotest 087: notrun if null-co is disabled...
Checking PATCH 12/17: iotest 093: notrun if null-co or null-aio is disabled...
Checking PATCH 13/17: iotest 099: notrun if blkdebug or blkverify is disabled...
Checking PATCH 14/17: iotest 124: skip testcases using blkdebug if disabled...
Checking PATCH 15/17: iotest 139: skip testcases using disabled drivers...
Checking PATCH 16/17: iotest 147: notrun if nbd is disabled...
Checking PATCH 17/17: iotest 184: notrun if null-co or throttle is disabled...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Thu, Apr 26, 2018 at 07:19:41PM +0300, Roman Kagan wrote: > Some iotests assume availability of certain block drivers, and fail if > the driver is not supported by QEMU because it was disabled at configure > time. > > This series tries to address that, by making QEMU report the actual list > of supported block drivers in response to "-drive format=?", and using > this information to skip the parts of the io testsuite that can not be > run in this configuration. > > Roman Kagan (17): > block: iterate_format with account of whitelisting > iotests: iotests.py: prevent deadlock in subprocess > iotests: ask qemu for supported formats > iotest 030: skip quorum test setup/teardown too > iotest 030: require blkdebug > iotest 055: skip unsupported backup target formats > iotest 055: require blkdebug > iotest 056: skip testcases using blkdebug if disabled > iotest 071: notrun if blkdebug or blkverify is disabled > iotest 081: notrun if quorum is disabled > iotest 087: notrun if null-co is disabled > iotest 093: notrun if null-co or null-aio is disabled > iotest 099: notrun if blkdebug or blkverify is disabled > iotest 124: skip testcases using blkdebug if disabled > iotest 139: skip testcases using disabled drivers > iotest 147: notrun if nbd is disabled > iotest 184: notrun if null-co or throttle is disabled > > include/block/block.h | 2 +- > block.c | 23 ++++++++++++++++++---- > blockdev.c | 4 +++- > qemu-img.c | 2 +- > tests/qemu-iotests/030 | 7 +++++++ > tests/qemu-iotests/055 | 13 ++++++++++++ > tests/qemu-iotests/056 | 3 +++ > tests/qemu-iotests/071 | 1 + > tests/qemu-iotests/081 | 1 + > tests/qemu-iotests/087 | 1 + > tests/qemu-iotests/093 | 1 + > tests/qemu-iotests/099 | 1 + > tests/qemu-iotests/124 | 5 +++++ > tests/qemu-iotests/139 | 4 ++++ > tests/qemu-iotests/147 | 1 + > tests/qemu-iotests/184 | 1 + > tests/qemu-iotests/common.rc | 19 ++++++++++++++++++ > tests/qemu-iotests/iotests.py | 46 ++++++++++++++++++++++++++++++++----------- > 18 files changed, 117 insertions(+), 18 deletions(-) Ping?
On 2018-04-26 18:19, Roman Kagan wrote: > Some iotests assume availability of certain block drivers, and fail if > the driver is not supported by QEMU because it was disabled at configure > time. > > This series tries to address that, by making QEMU report the actual list > of supported block drivers in response to "-drive format=?", and using > this information to skip the parts of the io testsuite that can not be > run in this configuration. > > Roman Kagan (17): > block: iterate_format with account of whitelisting > iotests: iotests.py: prevent deadlock in subprocess > iotests: ask qemu for supported formats > iotest 030: skip quorum test setup/teardown too > iotest 030: require blkdebug > iotest 055: skip unsupported backup target formats > iotest 055: require blkdebug > iotest 056: skip testcases using blkdebug if disabled > iotest 071: notrun if blkdebug or blkverify is disabled > iotest 081: notrun if quorum is disabled > iotest 087: notrun if null-co is disabled > iotest 093: notrun if null-co or null-aio is disabled > iotest 099: notrun if blkdebug or blkverify is disabled > iotest 124: skip testcases using blkdebug if disabled > iotest 139: skip testcases using disabled drivers > iotest 147: notrun if nbd is disabled > iotest 184: notrun if null-co or throttle is disabled > > include/block/block.h | 2 +- > block.c | 23 ++++++++++++++++++---- > blockdev.c | 4 +++- > qemu-img.c | 2 +- > tests/qemu-iotests/030 | 7 +++++++ > tests/qemu-iotests/055 | 13 ++++++++++++ > tests/qemu-iotests/056 | 3 +++ > tests/qemu-iotests/071 | 1 + > tests/qemu-iotests/081 | 1 + > tests/qemu-iotests/087 | 1 + > tests/qemu-iotests/093 | 1 + > tests/qemu-iotests/099 | 1 + > tests/qemu-iotests/124 | 5 +++++ > tests/qemu-iotests/139 | 4 ++++ > tests/qemu-iotests/147 | 1 + > tests/qemu-iotests/184 | 1 + > tests/qemu-iotests/common.rc | 19 ++++++++++++++++++ > tests/qemu-iotests/iotests.py | 46 ++++++++++++++++++++++++++++++++----------- > 18 files changed, 117 insertions(+), 18 deletions(-) I'll stop reviewing this series for now, because there are more iotests that use drivers outside of their format/protocol combination. For instance: $ grep -l null-co ??? | wc -l 15 $ grep -l blkdebug ??? | wc -l 30 $ (grep -l '"raw"' ???; grep -l "'raw'" ???) | wc -l 22 As I've written in my reply to patch 8, I'm not sure whether it's the right solution to check for the availability of these block drivers in every single test that needs them. It makes sense for quorum, because quorum needs an external library for hashing, so it may not be trivial to enable. But it does not seem too useful for other formats that do not have such a dependency (e.g. null-co, blkdebug, raw). The thing is that it's OK to whitelist everything for testing, and then disable some drivers when building a release. I don't think one needs to run the iotests with the release version if the whole difference is whether some drivers have been disabled or not. Max
On Wed, May 30, 2018 at 02:35:34PM +0200, Max Reitz wrote: > On 2018-04-26 18:19, Roman Kagan wrote: > > Some iotests assume availability of certain block drivers, and fail if > > the driver is not supported by QEMU because it was disabled at configure > > time. > > > > This series tries to address that, by making QEMU report the actual list > > of supported block drivers in response to "-drive format=?", and using > > this information to skip the parts of the io testsuite that can not be > > run in this configuration. > > > > Roman Kagan (17): > > block: iterate_format with account of whitelisting > > iotests: iotests.py: prevent deadlock in subprocess > > iotests: ask qemu for supported formats > > iotest 030: skip quorum test setup/teardown too > > iotest 030: require blkdebug > > iotest 055: skip unsupported backup target formats > > iotest 055: require blkdebug > > iotest 056: skip testcases using blkdebug if disabled > > iotest 071: notrun if blkdebug or blkverify is disabled > > iotest 081: notrun if quorum is disabled > > iotest 087: notrun if null-co is disabled > > iotest 093: notrun if null-co or null-aio is disabled > > iotest 099: notrun if blkdebug or blkverify is disabled > > iotest 124: skip testcases using blkdebug if disabled > > iotest 139: skip testcases using disabled drivers > > iotest 147: notrun if nbd is disabled > > iotest 184: notrun if null-co or throttle is disabled > > > > include/block/block.h | 2 +- > > block.c | 23 ++++++++++++++++++---- > > blockdev.c | 4 +++- > > qemu-img.c | 2 +- > > tests/qemu-iotests/030 | 7 +++++++ > > tests/qemu-iotests/055 | 13 ++++++++++++ > > tests/qemu-iotests/056 | 3 +++ > > tests/qemu-iotests/071 | 1 + > > tests/qemu-iotests/081 | 1 + > > tests/qemu-iotests/087 | 1 + > > tests/qemu-iotests/093 | 1 + > > tests/qemu-iotests/099 | 1 + > > tests/qemu-iotests/124 | 5 +++++ > > tests/qemu-iotests/139 | 4 ++++ > > tests/qemu-iotests/147 | 1 + > > tests/qemu-iotests/184 | 1 + > > tests/qemu-iotests/common.rc | 19 ++++++++++++++++++ > > tests/qemu-iotests/iotests.py | 46 ++++++++++++++++++++++++++++++++----------- > > 18 files changed, 117 insertions(+), 18 deletions(-) > > I'll stop reviewing this series for now, because there are more iotests > that use drivers outside of their format/protocol combination. > > For instance: > > $ grep -l null-co ??? | wc -l > 15 > $ grep -l blkdebug ??? | wc -l > 30 > $ (grep -l '"raw"' ???; grep -l "'raw'" ???) | wc -l > 22 > > As I've written in my reply to patch 8, I'm not sure whether it's the > right solution to check for the availability of these block drivers in > every single test that needs them. It makes sense for quorum, because > quorum needs an external library for hashing, so it may not be trivial > to enable. But it does not seem too useful for other formats that do > not have such a dependency (e.g. null-co, blkdebug, raw). > > The thing is that it's OK to whitelist everything for testing, and then > disable some drivers when building a release. I don't think one needs > to run the iotests with the release version if the whole difference is > whether some drivers have been disabled or not. This patchset was created when we started to run (quick) iotests as a part of the package build, which looked like a decent alternative to building separate CI infrastructure. If there's no general interest in running iotests against QEMU built in a release configuration with certain drivers blacklisted, I think we should be fine handling this locally. Thanks, Roman.
On 2018-05-30 15:47, Roman Kagan wrote: > On Wed, May 30, 2018 at 02:35:34PM +0200, Max Reitz wrote: >> On 2018-04-26 18:19, Roman Kagan wrote: >>> Some iotests assume availability of certain block drivers, and fail if >>> the driver is not supported by QEMU because it was disabled at configure >>> time. >>> >>> This series tries to address that, by making QEMU report the actual list >>> of supported block drivers in response to "-drive format=?", and using >>> this information to skip the parts of the io testsuite that can not be >>> run in this configuration. >>> >>> Roman Kagan (17): >>> block: iterate_format with account of whitelisting >>> iotests: iotests.py: prevent deadlock in subprocess >>> iotests: ask qemu for supported formats >>> iotest 030: skip quorum test setup/teardown too >>> iotest 030: require blkdebug >>> iotest 055: skip unsupported backup target formats >>> iotest 055: require blkdebug >>> iotest 056: skip testcases using blkdebug if disabled >>> iotest 071: notrun if blkdebug or blkverify is disabled >>> iotest 081: notrun if quorum is disabled >>> iotest 087: notrun if null-co is disabled >>> iotest 093: notrun if null-co or null-aio is disabled >>> iotest 099: notrun if blkdebug or blkverify is disabled >>> iotest 124: skip testcases using blkdebug if disabled >>> iotest 139: skip testcases using disabled drivers >>> iotest 147: notrun if nbd is disabled >>> iotest 184: notrun if null-co or throttle is disabled >>> >>> include/block/block.h | 2 +- >>> block.c | 23 ++++++++++++++++++---- >>> blockdev.c | 4 +++- >>> qemu-img.c | 2 +- >>> tests/qemu-iotests/030 | 7 +++++++ >>> tests/qemu-iotests/055 | 13 ++++++++++++ >>> tests/qemu-iotests/056 | 3 +++ >>> tests/qemu-iotests/071 | 1 + >>> tests/qemu-iotests/081 | 1 + >>> tests/qemu-iotests/087 | 1 + >>> tests/qemu-iotests/093 | 1 + >>> tests/qemu-iotests/099 | 1 + >>> tests/qemu-iotests/124 | 5 +++++ >>> tests/qemu-iotests/139 | 4 ++++ >>> tests/qemu-iotests/147 | 1 + >>> tests/qemu-iotests/184 | 1 + >>> tests/qemu-iotests/common.rc | 19 ++++++++++++++++++ >>> tests/qemu-iotests/iotests.py | 46 ++++++++++++++++++++++++++++++++----------- >>> 18 files changed, 117 insertions(+), 18 deletions(-) >> >> I'll stop reviewing this series for now, because there are more iotests >> that use drivers outside of their format/protocol combination. >> >> For instance: >> >> $ grep -l null-co ??? | wc -l >> 15 >> $ grep -l blkdebug ??? | wc -l >> 30 >> $ (grep -l '"raw"' ???; grep -l "'raw'" ???) | wc -l >> 22 >> >> As I've written in my reply to patch 8, I'm not sure whether it's the >> right solution to check for the availability of these block drivers in >> every single test that needs them. It makes sense for quorum, because >> quorum needs an external library for hashing, so it may not be trivial >> to enable. But it does not seem too useful for other formats that do >> not have such a dependency (e.g. null-co, blkdebug, raw). >> >> The thing is that it's OK to whitelist everything for testing, and then >> disable some drivers when building a release. I don't think one needs >> to run the iotests with the release version if the whole difference is >> whether some drivers have been disabled or not. > > This patchset was created when we started to run (quick) iotests as a part > of the package build, which looked like a decent alternative to building > separate CI infrastructure. > > If there's no general interest in running iotests against QEMU built in > a release configuration with certain drivers blacklisted, I think we > should be fine handling this locally. It's not like the majority of drivers uses blkdebug, but still, it seems at least suboptimal to run the tests if you then disable a big chunk of them because some drivers were not whitelisted. If you're OK with a local solution... OK, but naively I'd think that it'd be better to just build two versions for packaging: One that's actually going to be packaged, and one that's used for testing. (Though I don't know how well that fits into your process.) In any case, I'm not fully opposed to this series, but it would need to be complete and make every test require every block driver that is not part of its protocol/format combination. Max
On Wed, May 30, 2018 at 03:53:52PM +0200, Max Reitz wrote: > On 2018-05-30 15:47, Roman Kagan wrote: > > On Wed, May 30, 2018 at 02:35:34PM +0200, Max Reitz wrote: > >> On 2018-04-26 18:19, Roman Kagan wrote: > >>> Some iotests assume availability of certain block drivers, and fail if > >>> the driver is not supported by QEMU because it was disabled at configure > >>> time. > >>> > >>> This series tries to address that, by making QEMU report the actual list > >>> of supported block drivers in response to "-drive format=?", and using > >>> this information to skip the parts of the io testsuite that can not be > >>> run in this configuration. > >>> > >>> Roman Kagan (17): > >>> block: iterate_format with account of whitelisting > >>> iotests: iotests.py: prevent deadlock in subprocess > >>> iotests: ask qemu for supported formats > >>> iotest 030: skip quorum test setup/teardown too > >>> iotest 030: require blkdebug > >>> iotest 055: skip unsupported backup target formats > >>> iotest 055: require blkdebug > >>> iotest 056: skip testcases using blkdebug if disabled > >>> iotest 071: notrun if blkdebug or blkverify is disabled > >>> iotest 081: notrun if quorum is disabled > >>> iotest 087: notrun if null-co is disabled > >>> iotest 093: notrun if null-co or null-aio is disabled > >>> iotest 099: notrun if blkdebug or blkverify is disabled > >>> iotest 124: skip testcases using blkdebug if disabled > >>> iotest 139: skip testcases using disabled drivers > >>> iotest 147: notrun if nbd is disabled > >>> iotest 184: notrun if null-co or throttle is disabled > >>> > >>> include/block/block.h | 2 +- > >>> block.c | 23 ++++++++++++++++++---- > >>> blockdev.c | 4 +++- > >>> qemu-img.c | 2 +- > >>> tests/qemu-iotests/030 | 7 +++++++ > >>> tests/qemu-iotests/055 | 13 ++++++++++++ > >>> tests/qemu-iotests/056 | 3 +++ > >>> tests/qemu-iotests/071 | 1 + > >>> tests/qemu-iotests/081 | 1 + > >>> tests/qemu-iotests/087 | 1 + > >>> tests/qemu-iotests/093 | 1 + > >>> tests/qemu-iotests/099 | 1 + > >>> tests/qemu-iotests/124 | 5 +++++ > >>> tests/qemu-iotests/139 | 4 ++++ > >>> tests/qemu-iotests/147 | 1 + > >>> tests/qemu-iotests/184 | 1 + > >>> tests/qemu-iotests/common.rc | 19 ++++++++++++++++++ > >>> tests/qemu-iotests/iotests.py | 46 ++++++++++++++++++++++++++++++++----------- > >>> 18 files changed, 117 insertions(+), 18 deletions(-) > >> > >> I'll stop reviewing this series for now, because there are more iotests > >> that use drivers outside of their format/protocol combination. > >> > >> For instance: > >> > >> $ grep -l null-co ??? | wc -l > >> 15 > >> $ grep -l blkdebug ??? | wc -l > >> 30 > >> $ (grep -l '"raw"' ???; grep -l "'raw'" ???) | wc -l > >> 22 > >> > >> As I've written in my reply to patch 8, I'm not sure whether it's the > >> right solution to check for the availability of these block drivers in > >> every single test that needs them. It makes sense for quorum, because > >> quorum needs an external library for hashing, so it may not be trivial > >> to enable. But it does not seem too useful for other formats that do > >> not have such a dependency (e.g. null-co, blkdebug, raw). > >> > >> The thing is that it's OK to whitelist everything for testing, and then > >> disable some drivers when building a release. I don't think one needs > >> to run the iotests with the release version if the whole difference is > >> whether some drivers have been disabled or not. > > > > This patchset was created when we started to run (quick) iotests as a part > > of the package build, which looked like a decent alternative to building > > separate CI infrastructure. > > > > If there's no general interest in running iotests against QEMU built in > > a release configuration with certain drivers blacklisted, I think we > > should be fine handling this locally. > > It's not like the majority of drivers uses blkdebug, but still, it seems > at least suboptimal to run the tests if you then disable a big chunk of > them because some drivers were not whitelisted. Indeed. In fact, our configuration (based on RedHat's) has those three (null-co, blkdebug, raw) enabled so the patchset didn't result in massive test skips. > If you're OK with a local solution... OK, but naively I'd think that > it'd be better to just build two versions for packaging: One that's > actually going to be packaged, and one that's used for testing. (Though > I don't know how well that fits into your process.) It doesn't: we just added a couple of lines to the %check section of the RPM .spec > In any case, I'm not fully opposed to this series, but it would need to > be complete and make every test require every block driver that is not > part of its protocol/format combination. OK this doesn't look like too much of work; I'll see if I can do it quickly. Thanks, Roman.