* [Qemu-devel] [PATCH] qemu-iotests: fix cleanup of background processes
@ 2015-10-29 1:15 Jeff Cody
2015-10-29 4:08 ` Eric Blake
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Cody @ 2015-10-29 1:15 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf, jsnow, qemu-block, mreitz
Commit 934659c switched the iotests to run qemu and qemu-nbd from a bash
subshell, in order to catch segfaults. Unfortunately, this means the
process PID cannot be captured via '$!'. We stopped killing qemu and
qemu-nbd processes, leaving a lot of orphaned, running qemu processes
after executing iotests.
Since the process is using exec in the subshell, the PID is the
same as the subshell PID.
Track these PIDs for cleanup using pidfiles in the $TEST_DIR. Only
track the qemu PID, however, if requested - not all usage requires
killing the process.
Reported-by: John Snow <jsnow@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
tests/qemu-iotests/common.config | 14 ++++++++++++--
tests/qemu-iotests/common.qemu | 17 +++++++++++------
tests/qemu-iotests/common.rc | 6 +++---
3 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index 596bb2b..5fd4ca8 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -44,6 +44,8 @@ export HOST_OPTIONS=${HOST_OPTIONS:=local.config}
export CHECK_OPTIONS=${CHECK_OPTIONS:="-g auto"}
export PWD=`pwd`
+export _QEMU_HANDLE=0
+
# $1 = prog to look for, $2* = default pathnames if not found in $PATH
set_prog_path()
{
@@ -105,7 +107,12 @@ fi
_qemu_wrapper()
{
- (exec "$QEMU_PROG" $QEMU_OPTIONS "$@")
+ (
+ if [ ! -z ${QEMU_NEED_PID} ]; then
+ echo -n $BASHPID > "${TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
+ fi
+ exec "$QEMU_PROG" $QEMU_OPTIONS "$@"
+ )
}
_qemu_img_wrapper()
@@ -120,7 +127,10 @@ _qemu_io_wrapper()
_qemu_nbd_wrapper()
{
- (exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@")
+ (
+ echo -n $BASHPID > "${TEST_DIR}/qemu-nbd.pid"
+ exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
+ )
}
export QEMU=_qemu_wrapper
diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu
index e3faa53..bb108bd 100644
--- a/tests/qemu-iotests/common.qemu
+++ b/tests/qemu-iotests/common.qemu
@@ -30,8 +30,6 @@ QEMU_COMM_TIMEOUT=10
QEMU_FIFO_IN="${TEST_DIR}/qmp-in-$$"
QEMU_FIFO_OUT="${TEST_DIR}/qmp-out-$$"
-QEMU_PID=
-_QEMU_HANDLE=0
QEMU_HANDLE=0
# If bash version is >= 4.1, these will be overwritten and dynamic
@@ -153,11 +151,11 @@ function _launch_qemu()
mkfifo "${fifo_out}"
mkfifo "${fifo_in}"
+ QEMU_NEED_PID='y'\
${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \
>"${fifo_out}" \
2>&1 \
<"${fifo_in}" &
- QEMU_PID[${_QEMU_HANDLE}]=$!
if [[ "${BASH_VERSINFO[0]}" -ge "5" ||
("${BASH_VERSINFO[0]}" -ge "4" && "${BASH_VERSINFO[1]}" -ge "1") ]]
@@ -196,10 +194,17 @@ function _cleanup_qemu()
# QEMU_PID[], QEMU_IN[], QEMU_OUT[] all use same indices
for i in "${!QEMU_OUT[@]}"
do
- if [ -z "${wait}" ]; then
- kill -KILL ${QEMU_PID[$i]} 2>/dev/null
+ local QEMU_PID
+ if [ -f "${TEST_DIR}/qemu-${i}.pid" ]; then
+ QEMU_PID=`cat "${TEST_DIR}/qemu-${i}.pid"`
+ rm -f "${TEST_DIR}/qemu-${i}.pid"
+ fi
+ if [ -z "${wait}" ] && [ ! -z ${QEMU_PID} ]; then
+ kill -KILL ${QEMU_PID} 2>/dev/null
+ fi
+ if [ ! -z ${QEMU_PID} ]; then
+ wait ${QEMU_PID} 2>/dev/null # silent kill
fi
- wait ${QEMU_PID[$i]} 2>/dev/null # silent kill
if [ -n "${wait}" ]; then
cat <&${QEMU_OUT[$i]} | _filter_testdir | _filter_qemu \
| _filter_qemu_io | _filter_qmp
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 28e4bea..84d2e98 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -154,7 +154,6 @@ _make_test_img()
# Start an NBD server on the image file, which is what we'll be talking to
if [ $IMGPROTO = "nbd" ]; then
eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT $TEST_IMG_FILE &"
- QEMU_NBD_PID=$!
sleep 1 # FIXME: qemu-nbd needs to be listening before we continue
fi
}
@@ -175,8 +174,9 @@ _cleanup_test_img()
case "$IMGPROTO" in
nbd)
- if [ -n "$QEMU_NBD_PID" ]; then
- kill $QEMU_NBD_PID
+ if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then
+ kill `cat "${TEST_DIR}/qemu-nbd.pid"`
+ rm -f "${TEST_DIR}/qemu-nbd.pid"
fi
rm -f "$TEST_IMG_FILE"
;;
--
1.9.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-iotests: fix cleanup of background processes
2015-10-29 1:15 [Qemu-devel] [PATCH] qemu-iotests: fix cleanup of background processes Jeff Cody
@ 2015-10-29 4:08 ` Eric Blake
2015-10-29 11:21 ` Jeff Cody
0 siblings, 1 reply; 3+ messages in thread
From: Eric Blake @ 2015-10-29 4:08 UTC (permalink / raw)
To: Jeff Cody, qemu-devel; +Cc: kwolf, jsnow, qemu-block, mreitz
[-- Attachment #1: Type: text/plain, Size: 2945 bytes --]
On 10/28/2015 07:15 PM, Jeff Cody wrote:
> Commit 934659c switched the iotests to run qemu and qemu-nbd from a bash
> subshell, in order to catch segfaults. Unfortunately, this means the
> process PID cannot be captured via '$!'. We stopped killing qemu and
> qemu-nbd processes, leaving a lot of orphaned, running qemu processes
> after executing iotests.
>
> Since the process is using exec in the subshell, the PID is the
> same as the subshell PID.
>
> Track these PIDs for cleanup using pidfiles in the $TEST_DIR. Only
> track the qemu PID, however, if requested - not all usage requires
> killing the process.
>
> Reported-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
> tests/qemu-iotests/common.config | 14 ++++++++++++--
> tests/qemu-iotests/common.qemu | 17 +++++++++++------
> tests/qemu-iotests/common.rc | 6 +++---
> 3 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
> index 596bb2b..5fd4ca8 100644
> --- a/tests/qemu-iotests/common.config
> +++ b/tests/qemu-iotests/common.config
> @@ -44,6 +44,8 @@ export HOST_OPTIONS=${HOST_OPTIONS:=local.config}
> export CHECK_OPTIONS=${CHECK_OPTIONS:="-g auto"}
> export PWD=`pwd`
>
> +export _QEMU_HANDLE=0
> +
> # $1 = prog to look for, $2* = default pathnames if not found in $PATH
> set_prog_path()
> {
> @@ -105,7 +107,12 @@ fi
>
> _qemu_wrapper()
> {
> - (exec "$QEMU_PROG" $QEMU_OPTIONS "$@")
> + (
> + if [ ! -z ${QEMU_NEED_PID} ]; then
> + echo -n $BASHPID > "${TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
'echo -n' is a non-portable bashism; even in bash, it can be made to
behave differently by 'set -o posix; shopt -s xpg_echo'. It's safer,
and shorter, to use 'printf', if you don't need the newline.
On the other hand, if you use plain 'echo', and include the newline,...
> @@ -196,10 +194,17 @@ function _cleanup_qemu()
> # QEMU_PID[], QEMU_IN[], QEMU_OUT[] all use same indices
> for i in "${!QEMU_OUT[@]}"
> do
> - if [ -z "${wait}" ]; then
> - kill -KILL ${QEMU_PID[$i]} 2>/dev/null
> + local QEMU_PID
> + if [ -f "${TEST_DIR}/qemu-${i}.pid" ]; then
> + QEMU_PID=`cat "${TEST_DIR}/qemu-${i}.pid"`
...then you could avoid the subshell and useless use of cat here by doing:
read QEMU_PID < "${TEST_DIR}/qemu-${i}.pid"
> + rm -f "${TEST_DIR}/qemu-${i}.pid"
> + fi
> + if [ -z "${wait}" ] && [ ! -z ${QEMU_PID} ]; then
Missing quotes around ${QEMU_PID}. But you got lucky: if it is empty,
then you are evaluating [ ! -z ], which is false; where the intended [ !
-z "" ] would also be false. Still, it's bad form to abuse [] like that.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH] qemu-iotests: fix cleanup of background processes
2015-10-29 4:08 ` Eric Blake
@ 2015-10-29 11:21 ` Jeff Cody
0 siblings, 0 replies; 3+ messages in thread
From: Jeff Cody @ 2015-10-29 11:21 UTC (permalink / raw)
To: Eric Blake; +Cc: kwolf, jsnow, qemu-devel, qemu-block, mreitz
On Wed, Oct 28, 2015 at 10:08:42PM -0600, Eric Blake wrote:
> On 10/28/2015 07:15 PM, Jeff Cody wrote:
> > Commit 934659c switched the iotests to run qemu and qemu-nbd from a bash
> > subshell, in order to catch segfaults. Unfortunately, this means the
> > process PID cannot be captured via '$!'. We stopped killing qemu and
> > qemu-nbd processes, leaving a lot of orphaned, running qemu processes
> > after executing iotests.
> >
> > Since the process is using exec in the subshell, the PID is the
> > same as the subshell PID.
> >
> > Track these PIDs for cleanup using pidfiles in the $TEST_DIR. Only
> > track the qemu PID, however, if requested - not all usage requires
> > killing the process.
> >
> > Reported-by: John Snow <jsnow@redhat.com>
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > ---
> > tests/qemu-iotests/common.config | 14 ++++++++++++--
> > tests/qemu-iotests/common.qemu | 17 +++++++++++------
> > tests/qemu-iotests/common.rc | 6 +++---
> > 3 files changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
> > index 596bb2b..5fd4ca8 100644
> > --- a/tests/qemu-iotests/common.config
> > +++ b/tests/qemu-iotests/common.config
> > @@ -44,6 +44,8 @@ export HOST_OPTIONS=${HOST_OPTIONS:=local.config}
> > export CHECK_OPTIONS=${CHECK_OPTIONS:="-g auto"}
> > export PWD=`pwd`
> >
> > +export _QEMU_HANDLE=0
> > +
> > # $1 = prog to look for, $2* = default pathnames if not found in $PATH
> > set_prog_path()
> > {
> > @@ -105,7 +107,12 @@ fi
> >
> > _qemu_wrapper()
> > {
> > - (exec "$QEMU_PROG" $QEMU_OPTIONS "$@")
> > + (
> > + if [ ! -z ${QEMU_NEED_PID} ]; then
> > + echo -n $BASHPID > "${TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
>
> 'echo -n' is a non-portable bashism; even in bash, it can be made to
> behave differently by 'set -o posix; shopt -s xpg_echo'. It's safer,
> and shorter, to use 'printf', if you don't need the newline.
>
> On the other hand, if you use plain 'echo', and include the newline,...
>
> > @@ -196,10 +194,17 @@ function _cleanup_qemu()
> > # QEMU_PID[], QEMU_IN[], QEMU_OUT[] all use same indices
> > for i in "${!QEMU_OUT[@]}"
> > do
> > - if [ -z "${wait}" ]; then
> > - kill -KILL ${QEMU_PID[$i]} 2>/dev/null
> > + local QEMU_PID
> > + if [ -f "${TEST_DIR}/qemu-${i}.pid" ]; then
> > + QEMU_PID=`cat "${TEST_DIR}/qemu-${i}.pid"`
>
> ...then you could avoid the subshell and useless use of cat here by doing:
>
> read QEMU_PID < "${TEST_DIR}/qemu-${i}.pid"
>
Yes, that would be better.
> > + rm -f "${TEST_DIR}/qemu-${i}.pid"
> > + fi
> > + if [ -z "${wait}" ] && [ ! -z ${QEMU_PID} ]; then
>
> Missing quotes around ${QEMU_PID}. But you got lucky: if it is empty,
> then you are evaluating [ ! -z ], which is false; where the intended [ !
> -z "" ] would also be false. Still, it's bad form to abuse [] like that.
>
Good catch, thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-10-29 11:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-29 1:15 [Qemu-devel] [PATCH] qemu-iotests: fix cleanup of background processes Jeff Cody
2015-10-29 4:08 ` Eric Blake
2015-10-29 11:21 ` Jeff Cody
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).