From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58870) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZsDcW-0004F7-Ul for qemu-devel@nongnu.org; Fri, 30 Oct 2015 13:37:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZsDcV-0005E6-NR for qemu-devel@nongnu.org; Fri, 30 Oct 2015 13:37:44 -0400 References: <271f1d0bbbe9cdfce39e4bfa481f88b4a03001c9.1446119717.git.jcody@redhat.com> From: Max Reitz Message-ID: <5633AADE.8080402@redhat.com> Date: Fri, 30 Oct 2015 18:37:34 +0100 MIME-Version: 1.0 In-Reply-To: <271f1d0bbbe9cdfce39e4bfa481f88b4a03001c9.1446119717.git.jcody@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="LR899BW545MfhCr2pImexCqQV1HnfgAmi" 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: Jeff Cody , qemu-devel@nongnu.org Cc: kwolf@redhat.com, jsnow@redhat.com, eblake@hdredirect-lb-399551664.us-east-1.elb.amazonaws.com, qemu-block@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --LR899BW545MfhCr2pImexCqQV1HnfgAmi Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 29.10.2015 12:56, Jeff Cody wrote: > Commit 934659c switched the iotests to run qemu and qemu-nbd from a bas= h > 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. >=20 > Since the process is using exec in the subshell, the PID is the > same as the subshell PID. >=20 > 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. >=20 > 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(-) >=20 > diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/comm= on.config > index 596bb2b..4d8665f 100644 > --- a/tests/qemu-iotests/common.config > +++ b/tests/qemu-iotests/common.config > @@ -44,6 +44,8 @@ export HOST_OPTIONS=3D${HOST_OPTIONS:=3Dlocal.config}= > export CHECK_OPTIONS=3D${CHECK_OPTIONS:=3D"-g auto"} > export PWD=3D`pwd` > =20 > +export _QEMU_HANDLE=3D0 > + > # $1 =3D prog to look for, $2* =3D default pathnames if not found in $= PATH > set_prog_path() > { > @@ -105,7 +107,12 @@ fi > =20 > _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 "$@" > + ) > } > =20 > _qemu_img_wrapper() > @@ -120,7 +127,10 @@ _qemu_io_wrapper() > =20 > _qemu_nbd_wrapper() > { > - (exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@") > + ( > + echo $BASHPID > "${TEST_DIR}/qemu-nbd.pid" > + exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@" > + ) > } > =20 > export QEMU=3D_qemu_wrapper > diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common= =2Eqemu > index e3faa53..18ad9a6 100644 > --- a/tests/qemu-iotests/common.qemu > +++ b/tests/qemu-iotests/common.qemu > @@ -30,8 +30,6 @@ QEMU_COMM_TIMEOUT=3D10 > QEMU_FIFO_IN=3D"${TEST_DIR}/qmp-in-$$" > QEMU_FIFO_OUT=3D"${TEST_DIR}/qmp-out-$$" > =20 > -QEMU_PID=3D > -_QEMU_HANDLE=3D0 > QEMU_HANDLE=3D0 > =20 > # If bash version is >=3D 4.1, these will be overwritten and dynamic > @@ -153,11 +151,11 @@ function _launch_qemu() > mkfifo "${fifo_out}" > mkfifo "${fifo_in}" > =20 > + QEMU_NEED_PID=3D'y'\ > ${QEMU} -nographic -serial none ${comm} -machine accel=3Dqtest "${= @}" \ > >"${fi= fo_out}" \ > 2>&1 \= > <"${fi= fo_in}" & > - QEMU_PID[${_QEMU_HANDLE}]=3D$! > =20 > 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.r= c > 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 ta= lking to > if [ $IMGPROTO =3D "nbd" ]; then > eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT $TEST_= IMG_FILE &" > - QEMU_NBD_PID=3D$! > sleep 1 # FIXME: qemu-nbd needs to be listening before we cont= inue > fi > } > @@ -175,8 +174,11 @@ _cleanup_test_img() > case "$IMGPROTO" in > =20 > 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" > ;; >=20 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 --LR899BW545MfhCr2pImexCqQV1HnfgAmi Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWM6reAAoJEDuxQgLoOKytm/YIAJZqlFG3oQWefSQi77/I/Vvi DLIiRzSMFmCussW7WULP+AlHdZ0PKdpi858d6AxO4K64bKa0ylc4IAHQ4vzuuFMu l2mP+g+FY7b+soP6GfqkkouJ8bCz4F8ymh1vhs9+iK2UZBgZL7gj0ciN47LOxLae h/NiQSIr1MKEZs26A1qqBOcZuhd2P3ZdAP7OskqenYwn2Y/Gb9gTfxbGT/+Ji6Dr cY3ckO1k49Nz2bsi2COvX975JMSDFj3QB+flDV+YGTsEe9aC0VotA38CBXQpiSHt ItQUx/mv3ePGN1BODLHOZaakpGFpTlwTmaJvxNLhwAbWPe0641pBk87xMmFlZTI= =Vb9F -----END PGP SIGNATURE----- --LR899BW545MfhCr2pImexCqQV1HnfgAmi--