| Message ID | 815765cfcea726c7a4cc619c4f6c3bedb47f16b9.1492537347.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/18/2017 12:45 PM, Jeff Cody wrote: > For the tests that use the common.qemu functions for running a QEMU > process, _cleanup_qemu must be called in the exit function. > > If it is not, if the qemu process aborts, then not all of the droppings > are cleaned up (e.g. pidfile, fifos). > > This updates those tests that did not have a cleanup in qemu-iotests. > > (I swapped spaces for tabs in test 102 as well) > > Reported-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > > v2 fixed typo in test 094, s/cleanup/_cleanup/ for the trap function. > > tests/qemu-iotests/028 | 1 + > tests/qemu-iotests/094 | 11 ++++++++--- > tests/qemu-iotests/102 | 5 +++-- > tests/qemu-iotests/109 | 1 + > tests/qemu-iotests/117 | 1 + > tests/qemu-iotests/130 | 1 + > tests/qemu-iotests/140 | 1 + > tests/qemu-iotests/141 | 1 + > tests/qemu-iotests/143 | 1 + > 9 files changed, 18 insertions(+), 5 deletions(-) Looks like you missed (at least?) 063
On Tue, Apr 18, 2017 at 01:44:43PM -0500, Eric Blake wrote: > On 04/18/2017 12:45 PM, Jeff Cody wrote: > > For the tests that use the common.qemu functions for running a QEMU > > process, _cleanup_qemu must be called in the exit function. > > > > If it is not, if the qemu process aborts, then not all of the droppings > > are cleaned up (e.g. pidfile, fifos). > > > > This updates those tests that did not have a cleanup in qemu-iotests. > > > > (I swapped spaces for tabs in test 102 as well) > > > > Reported-by: Eric Blake <eblake@redhat.com> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > > > v2 fixed typo in test 094, s/cleanup/_cleanup/ for the trap function. > > > > tests/qemu-iotests/028 | 1 + > > tests/qemu-iotests/094 | 11 ++++++++--- > > tests/qemu-iotests/102 | 5 +++-- > > tests/qemu-iotests/109 | 1 + > > tests/qemu-iotests/117 | 1 + > > tests/qemu-iotests/130 | 1 + > > tests/qemu-iotests/140 | 1 + > > tests/qemu-iotests/141 | 1 + > > tests/qemu-iotests/143 | 1 + > > 9 files changed, 18 insertions(+), 5 deletions(-) > > Looks like you missed (at least?) 063 > I did miss 156. But unless I am missing something, 063 does not use common.qemu... (I'll send a v3 with 156) -Jeff
On 04/18/2017 02:31 PM, Jeff Cody wrote: > On Tue, Apr 18, 2017 at 01:44:43PM -0500, Eric Blake wrote: >> On 04/18/2017 12:45 PM, Jeff Cody wrote: >>> For the tests that use the common.qemu functions for running a QEMU >>> process, _cleanup_qemu must be called in the exit function. >>> >>> If it is not, if the qemu process aborts, then not all of the droppings >>> are cleaned up (e.g. pidfile, fifos). >>> >> >> Looks like you missed (at least?) 063 >> > > I did miss 156. But unless I am missing something, 063 does not use > common.qemu... Good point. 063 does litter, even on success, however: $ rm -rf scratch/ $ ./check -raw 063 ... $ ls scratch/ t.raw.raw1 One idea presented in an earlier thread was that common.rc should create a subdirectory per test (rather than all tests sharing scratch/), and then common.rc itself install the cleanup hook that wipes out the entire subdirectory (or maybe even add a command-line option to ./check to suppress wiping when it is desirable to debug a test failure by seeing the droppings left behind).
On Tue, Apr 18, 2017 at 02:39:12PM -0500, Eric Blake wrote: > On 04/18/2017 02:31 PM, Jeff Cody wrote: > > On Tue, Apr 18, 2017 at 01:44:43PM -0500, Eric Blake wrote: > >> On 04/18/2017 12:45 PM, Jeff Cody wrote: > >>> For the tests that use the common.qemu functions for running a QEMU > >>> process, _cleanup_qemu must be called in the exit function. > >>> > >>> If it is not, if the qemu process aborts, then not all of the droppings > >>> are cleaned up (e.g. pidfile, fifos). > >>> > > >> > >> Looks like you missed (at least?) 063 > >> > > > > I did miss 156. But unless I am missing something, 063 does not use > > common.qemu... > > Good point. 063 does litter, even on success, however: > > $ rm -rf scratch/ > $ ./check -raw 063 > ... > $ ls scratch/ > t.raw.raw1 > > One idea presented in an earlier thread was that common.rc should create > a subdirectory per test (rather than all tests sharing scratch/), and > then common.rc itself install the cleanup hook that wipes out the entire > subdirectory (or maybe even add a command-line option to ./check to > suppress wiping when it is desirable to debug a test failure by seeing > the droppings left behind). > Yep, good ideas. I'll leave that for another series though; there are a few other changes based on some suggestions from Daniel that I'd like to implement as well, and I can fold your suggestions above into a new series (unless someone else does it first). -Jeff
On Tue, 04/18 15:42, Jeff Cody wrote: > > One idea presented in an earlier thread was that common.rc should create > > a subdirectory per test (rather than all tests sharing scratch/), and > > then common.rc itself install the cleanup hook that wipes out the entire > > subdirectory (or maybe even add a command-line option to ./check to > > suppress wiping when it is desirable to debug a test failure by seeing > > the droppings left behind). > > > > Yep, good ideas. I'll leave that for another series though; there are a few > other changes based on some suggestions from Daniel that I'd like to > implement as well, and I can fold your suggestions above into a new series > (unless someone else does it first). Sounds good, I was thinking about making ./check run tests in parallel. If each test has its own tmpdir, it will be much easier. Fam
On 19.04.2017 12:36, Fam Zheng wrote: > On Tue, 04/18 15:42, Jeff Cody wrote: >>> One idea presented in an earlier thread was that common.rc should create >>> a subdirectory per test (rather than all tests sharing scratch/), and >>> then common.rc itself install the cleanup hook that wipes out the entire >>> subdirectory (or maybe even add a command-line option to ./check to >>> suppress wiping when it is desirable to debug a test failure by seeing >>> the droppings left behind). >>> >> >> Yep, good ideas. I'll leave that for another series though; there are a few >> other changes based on some suggestions from Daniel that I'd like to >> implement as well, and I can fold your suggestions above into a new series >> (unless someone else does it first). > > Sounds good, I was thinking about making ./check run tests in parallel. If each > test has its own tmpdir, it will be much easier. In case you were not yet aware: you can manually set TEST_DIR, this is what I do. (I have a script that basically does TEST_DIR=/tmp/test-$1 ./check -$1) Max
On Wed, Apr 19, 2017 at 02:13:35PM +0200, Max Reitz wrote: > On 19.04.2017 12:36, Fam Zheng wrote: > > On Tue, 04/18 15:42, Jeff Cody wrote: > >>> One idea presented in an earlier thread was that common.rc should create > >>> a subdirectory per test (rather than all tests sharing scratch/), and > >>> then common.rc itself install the cleanup hook that wipes out the entire > >>> subdirectory (or maybe even add a command-line option to ./check to > >>> suppress wiping when it is desirable to debug a test failure by seeing > >>> the droppings left behind). > >>> > >> > >> Yep, good ideas. I'll leave that for another series though; there are a few > >> other changes based on some suggestions from Daniel that I'd like to > >> implement as well, and I can fold your suggestions above into a new series > >> (unless someone else does it first). > > > > Sounds good, I was thinking about making ./check run tests in parallel. If each > > test has its own tmpdir, it will be much easier. > Neat - it'd be cool to be able to use the same options as for 'make', e.g. ./check -j5. > In case you were not yet aware: you can manually set TEST_DIR, this is > what I do. > > (I have a script that basically does TEST_DIR=/tmp/test-$1 ./check -$1) > Baking it into common without relying on external env variables adds extra niceties, because as Eric mentioned cleanup is not left up to the script then, and the subdir can be nuked (or kept, via an option to 'check'). This would make the scripts simpler to read and implement, and less prone to do Bad Things. (I keep waiting for some iotest script to misquote a var during cleanup and do an rm -f ~/* or something... it'd be nice to not have to worry about that). -Jeff
On 19.04.2017 14:36, Jeff Cody wrote: > On Wed, Apr 19, 2017 at 02:13:35PM +0200, Max Reitz wrote: >> On 19.04.2017 12:36, Fam Zheng wrote: >>> On Tue, 04/18 15:42, Jeff Cody wrote: >>>>> One idea presented in an earlier thread was that common.rc should create >>>>> a subdirectory per test (rather than all tests sharing scratch/), and >>>>> then common.rc itself install the cleanup hook that wipes out the entire >>>>> subdirectory (or maybe even add a command-line option to ./check to >>>>> suppress wiping when it is desirable to debug a test failure by seeing >>>>> the droppings left behind). >>>>> >>>> >>>> Yep, good ideas. I'll leave that for another series though; there are a few >>>> other changes based on some suggestions from Daniel that I'd like to >>>> implement as well, and I can fold your suggestions above into a new series >>>> (unless someone else does it first). >>> >>> Sounds good, I was thinking about making ./check run tests in parallel. If each >>> test has its own tmpdir, it will be much easier. >> > > Neat - it'd be cool to be able to use the same options as for 'make', e.g. > ./check -j5. > > >> In case you were not yet aware: you can manually set TEST_DIR, this is >> what I do. >> >> (I have a script that basically does TEST_DIR=/tmp/test-$1 ./check -$1) >> > > Baking it into common without relying on external env variables adds extra > niceties, because as Eric mentioned cleanup is not left up to the script > then, and the subdir can be nuked (or kept, via an option to 'check'). > > This would make the scripts simpler to read and implement, and less prone to > do Bad Things. (I keep waiting for some iotest script to misquote a var > during cleanup and do an rm -f ~/* or something... it'd be nice to not have > to worry about that). Sure, I agree. I just wanted to throw in that it isn't completely impossible to run iotests in parallel, at least for different formats. (Baking it in would also mean that we could automatically distribute test invocations for a single format to multiple instances, yes (as in your ./check -j5 proposal). But that's work to do. :-)) Max
diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028 index 7783e57..97a8869 100755 --- a/tests/qemu-iotests/028 +++ b/tests/qemu-iotests/028 @@ -32,6 +32,7 @@ status=1 # failure is the default! _cleanup() { + _cleanup_qemu rm -f "${TEST_IMG}.copy" _cleanup_test_img } diff --git a/tests/qemu-iotests/094 b/tests/qemu-iotests/094 index 0ba0b0c..9aa01e3 100755 --- a/tests/qemu-iotests/094 +++ b/tests/qemu-iotests/094 @@ -27,7 +27,14 @@ echo "QA output created by $seq" here="$PWD" status=1 # failure is the default! -trap "exit \$status" 0 1 2 3 15 +_cleanup() +{ + _cleanup_qemu + _cleanup_test_img + rm -f "$TEST_DIR/source.$IMGFMT" +} + +trap "_cleanup; exit \$status" 0 1 2 3 15 # get standard environment, filters and checks . ./common.rc @@ -73,8 +80,6 @@ _send_qemu_cmd $QEMU_HANDLE \ wait=1 _cleanup_qemu -_cleanup_test_img -rm -f "$TEST_DIR/source.$IMGFMT" # success, all done echo '*** done' diff --git a/tests/qemu-iotests/102 b/tests/qemu-iotests/102 index 64b4af9..87db1bb 100755 --- a/tests/qemu-iotests/102 +++ b/tests/qemu-iotests/102 @@ -25,11 +25,12 @@ seq=$(basename $0) echo "QA output created by $seq" here=$PWD -status=1 # failure is the default! +status=1 # failure is the default! _cleanup() { - _cleanup_test_img + _cleanup_qemu + _cleanup_test_img } trap "_cleanup; exit \$status" 0 1 2 3 15 diff --git a/tests/qemu-iotests/109 b/tests/qemu-iotests/109 index f980b0c..4781387 100755 --- a/tests/qemu-iotests/109 +++ b/tests/qemu-iotests/109 @@ -29,6 +29,7 @@ status=1 # failure is the default! _cleanup() { + _cleanup_qemu rm -f $TEST_IMG.src _cleanup_test_img } diff --git a/tests/qemu-iotests/117 b/tests/qemu-iotests/117 index 9385b3f..9ebfc4b 100755 --- a/tests/qemu-iotests/117 +++ b/tests/qemu-iotests/117 @@ -29,6 +29,7 @@ status=1 # failure is the default! _cleanup() { + _cleanup_qemu _cleanup_test_img } trap "_cleanup; exit \$status" 0 1 2 3 15 diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130 index ecc8a5b..d4d148b 100755 --- a/tests/qemu-iotests/130 +++ b/tests/qemu-iotests/130 @@ -31,6 +31,7 @@ status=1 # failure is the default! _cleanup() { + _cleanup_qemu _cleanup_test_img } trap "_cleanup; exit \$status" 0 1 2 3 15 diff --git a/tests/qemu-iotests/140 b/tests/qemu-iotests/140 index 49f9df4..8c80a5a 100755 --- a/tests/qemu-iotests/140 +++ b/tests/qemu-iotests/140 @@ -33,6 +33,7 @@ status=1 # failure is the default! _cleanup() { + _cleanup_qemu _cleanup_test_img rm -f "$TEST_DIR/nbd" } diff --git a/tests/qemu-iotests/141 b/tests/qemu-iotests/141 index b2617e5..b1956d4 100755 --- a/tests/qemu-iotests/141 +++ b/tests/qemu-iotests/141 @@ -29,6 +29,7 @@ status=1 # failure is the default! _cleanup() { + _cleanup_qemu _cleanup_test_img rm -f "$TEST_DIR/{b,m,o}.$IMGFMT" } diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143 index ec4ef22..5ff1944 100755 --- a/tests/qemu-iotests/143 +++ b/tests/qemu-iotests/143 @@ -29,6 +29,7 @@ status=1 # failure is the default! _cleanup() { + _cleanup_qemu rm -f "$TEST_DIR/nbd" } trap "_cleanup; exit \$status" 0 1 2 3 15