From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34635) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZsDo9-0004lP-8F for qemu-devel@nongnu.org; Fri, 30 Oct 2015 13:49:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZsDo7-0000ev-U4 for qemu-devel@nongnu.org; Fri, 30 Oct 2015 13:49:45 -0400 Date: Fri, 30 Oct 2015 13:49:35 -0400 From: Jeff Cody Message-ID: <20151030174934.GC1600@localhost.localdomain> References: <271f1d0bbbe9cdfce39e4bfa481f88b4a03001c9.1446119717.git.jcody@redhat.com> <5633AADE.8080402@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5633AADE.8080402@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 1/1] qemu-iotests: fix cleanup of background processes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: kwolf@redhat.com, jsnow@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, eblake@hdredirect-lb-399551664.us-east-1.elb.amazonaws.com 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 > > Signed-off-by: Jeff Cody > > --- > > 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. > 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