All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit
@ 2017-04-18 17:45 Jeff Cody
  2017-04-18 18:44 ` Eric Blake
  0 siblings, 1 reply; 9+ messages in thread
From: Jeff Cody @ 2017-04-18 17:45 UTC (permalink / raw)
  To: qemu-devel; +Cc: qemu-block, kwolf, mreitz, eblake

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

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

* Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit
  2017-04-18 17:45 [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit Jeff Cody
@ 2017-04-18 18:44 ` Eric Blake
  2017-04-18 19:31   ` Jeff Cody
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2017-04-18 18:44 UTC (permalink / raw)
  To: Jeff Cody, qemu-devel; +Cc: qemu-block, kwolf, mreitz

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

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

* Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit
  2017-04-18 18:44 ` Eric Blake
@ 2017-04-18 19:31   ` Jeff Cody
  2017-04-18 19:39     ` Eric Blake
  0 siblings, 1 reply; 9+ messages in thread
From: Jeff Cody @ 2017-04-18 19:31 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel, qemu-block, kwolf, mreitz

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

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

* Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit
  2017-04-18 19:31   ` Jeff Cody
@ 2017-04-18 19:39     ` Eric Blake
  2017-04-18 19:42       ` Jeff Cody
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Blake @ 2017-04-18 19:39 UTC (permalink / raw)
  To: Jeff Cody; +Cc: qemu-devel, qemu-block, kwolf, mreitz

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

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

* Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit
  2017-04-18 19:39     ` Eric Blake
@ 2017-04-18 19:42       ` Jeff Cody
  2017-04-19 10:36         ` Fam Zheng
  0 siblings, 1 reply; 9+ messages in thread
From: Jeff Cody @ 2017-04-18 19:42 UTC (permalink / raw)
  To: Eric Blake; +Cc: qemu-devel, qemu-block, kwolf, mreitz

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

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

* Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit
  2017-04-18 19:42       ` Jeff Cody
@ 2017-04-19 10:36         ` Fam Zheng
  2017-04-19 12:13           ` Max Reitz
  0 siblings, 1 reply; 9+ messages in thread
From: Fam Zheng @ 2017-04-19 10:36 UTC (permalink / raw)
  To: Jeff Cody; +Cc: Eric Blake, kwolf, qemu-devel, qemu-block, mreitz

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

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

* Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit
  2017-04-19 10:36         ` Fam Zheng
@ 2017-04-19 12:13           ` Max Reitz
  2017-04-19 12:36             ` Jeff Cody
  0 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2017-04-19 12:13 UTC (permalink / raw)
  To: Fam Zheng, Jeff Cody; +Cc: Eric Blake, kwolf, qemu-devel, qemu-block

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

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

* Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit
  2017-04-19 12:13           ` Max Reitz
@ 2017-04-19 12:36             ` Jeff Cody
  2017-04-19 12:45               ` Max Reitz
  0 siblings, 1 reply; 9+ messages in thread
From: Jeff Cody @ 2017-04-19 12:36 UTC (permalink / raw)
  To: Max Reitz; +Cc: Fam Zheng, Eric Blake, kwolf, qemu-devel, qemu-block

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

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

* Re: [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit
  2017-04-19 12:36             ` Jeff Cody
@ 2017-04-19 12:45               ` Max Reitz
  0 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2017-04-19 12:45 UTC (permalink / raw)
  To: Jeff Cody; +Cc: Fam Zheng, Eric Blake, kwolf, qemu-devel, qemu-block

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

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

end of thread, other threads:[~2017-04-19 12:46 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-18 17:45 [Qemu-devel] [PATCH v2 for-2.10 1/1] qemu-iotests: _cleanup_qemu must be called on exit Jeff Cody
2017-04-18 18:44 ` Eric Blake
2017-04-18 19:31   ` Jeff Cody
2017-04-18 19:39     ` Eric Blake
2017-04-18 19:42       ` Jeff Cody
2017-04-19 10:36         ` Fam Zheng
2017-04-19 12:13           ` Max Reitz
2017-04-19 12:36             ` Jeff Cody
2017-04-19 12:45               ` Max Reitz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.