[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/1] qemu-iotests: fix cleanup of background
From: |
Jeff Cody |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/1] qemu-iotests: fix cleanup of background processes |
Date: |
Fri, 30 Oct 2015 13:49:35 -0400 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Fri, Oct 30, 2015 at 06:37:34PM +0100, Max Reitz wrote:
> On 29.10.2015 12:56, 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 <address@hidden>
> > Signed-off-by: Jeff Cody <address@hidden>
> > ---
> > tests/qemu-iotests/common.config | 14 ++++++++++++--
> > tests/qemu-iotests/common.qemu | 17 +++++++++++------
> > tests/qemu-iotests/common.rc | 8 +++++---
> > 3 files changed, 28 insertions(+), 11 deletions(-)
> >
> > diff --git a/tests/qemu-iotests/common.config
> > b/tests/qemu-iotests/common.config
> > index 596bb2b..4d8665f 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 $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 $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..18ad9a6 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
> > "address@hidden" \
> >
> > >"${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 "address@hidden"
> > do
> > - if [ -z "${wait}" ]; then
> > - kill -KILL ${QEMU_PID[$i]} 2>/dev/null
> > + local QEMU_PID
> > + if [ -f "${TEST_DIR}/qemu-${i}.pid" ]; then
> > + read QEMU_PID < "${TEST_DIR}/qemu-${i}.pid"
> > + rm -f "${TEST_DIR}/qemu-${i}.pid"
> > + fi
> > + if [ -z "${wait}" ] && [ ! -z "${QEMU_PID}" ]; then
>
> If $QEMU_PID has not been set in this iteration because qemu-${i}.pid
> did not exist, it will retain the value from the last iteration. So I
> suggest you either explicitly clear $QEMU_PID if qemu-${i}.pid does not
> exist, or move these two conditional blocks relying on the non-emptiness
> of $QEMU_PID into the [ -f "${TEST_DIR}/qemu-${i}.pid" ] block.
>
Good point. I'll just pull them up in the file check if block, as you
suggest.
> > + 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..4878e99 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,11 @@ _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
> > + local QEMU_NBD_PID
> > + read QEMU_NBD_PID < "${TEST_DIR}/qemu-nbd.pid"
> > + kill ${QEMU_NBD_PID}
> > + rm -f "${TEST_DIR}/qemu-nbd.pid"
> > fi
> > rm -f "$TEST_IMG_FILE"
> > ;;
> >
>
> Test 058 needs fixing, too, because it is clever enough to implement its
> own version of this (because it's not using nbd as the test protocol,
> but just running qemu-nbd on its own).
>
Sneaky! I'll add a patch to the series to fix that.
Thanks!
Jeff