[Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit

[Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit

[Jeff Cody]

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

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

Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit

[Jeff Cody]

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

Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit

[Jeff Cody]

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

Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

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


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

Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit

[Jeff Cody]

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

Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2181 bytes --]

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


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