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 <[email protected]> > Signed-off-by: Jeff Cody <[email protected]> > --- > 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 "${@}" \ > > >"${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 > + 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.
> + 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).
Max
signature.asc
Description: OpenPGP digital signature
