[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 1/1] qemu-iotests: fix cleanup of background
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v2 1/1] qemu-iotests: fix cleanup of background processes |
Date: |
Fri, 30 Oct 2015 18:37:34 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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.
> + 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