* [Qemu-devel] [PATCH v2 0/1] qemu-iotests: fix cleanup of background processes @ 2015-10-29 11:56 Jeff Cody 2015-10-29 11:56 ` [Qemu-devel] [PATCH v2 1/1] " Jeff Cody 0 siblings, 1 reply; 4+ messages in thread From: Jeff Cody @ 2015-10-29 11:56 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, jsnow, eblake, qemu-block, mreitz Changes from v1: * use 'read' instead of 'cat' (thanks Eric) * quote variable in variable test (thanks Eric) Jeff Cody (1): qemu-iotests: fix cleanup of background processes 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(-) -- 1.9.3 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Qemu-devel] [PATCH v2 1/1] qemu-iotests: fix cleanup of background processes 2015-10-29 11:56 [Qemu-devel] [PATCH v2 0/1] qemu-iotests: fix cleanup of background processes Jeff Cody @ 2015-10-29 11:56 ` Jeff Cody 2015-10-30 17:37 ` Max Reitz 0 siblings, 1 reply; 4+ messages in thread From: Jeff Cody @ 2015-10-29 11:56 UTC (permalink / raw) To: qemu-devel; +Cc: kwolf, jsnow, eblake, qemu-block, mreitz 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 <jsnow@redhat.com> Signed-off-by: Jeff Cody <jcody@redhat.com> --- 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 + 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" ;; -- 1.9.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] qemu-iotests: fix cleanup of background processes 2015-10-29 11:56 ` [Qemu-devel] [PATCH v2 1/1] " Jeff Cody @ 2015-10-30 17:37 ` Max Reitz 2015-10-30 17:49 ` Jeff Cody 0 siblings, 1 reply; 4+ messages in thread From: Max Reitz @ 2015-10-30 17:37 UTC (permalink / raw) To: Jeff Cody, qemu-devel; +Cc: kwolf, jsnow, eblake, qemu-block [-- Attachment #1: Type: text/plain, Size: 5756 bytes --] 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 <jsnow@redhat.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > 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 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/1] qemu-iotests: fix cleanup of background processes 2015-10-30 17:37 ` Max Reitz @ 2015-10-30 17:49 ` Jeff Cody 0 siblings, 0 replies; 4+ messages in thread From: Jeff Cody @ 2015-10-30 17:49 UTC (permalink / raw) To: Max Reitz; +Cc: kwolf, jsnow, qemu-devel, qemu-block, eblake 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 <jsnow@redhat.com> > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-10-30 17:49 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-29 11:56 [Qemu-devel] [PATCH v2 0/1] qemu-iotests: fix cleanup of background processes Jeff Cody 2015-10-29 11:56 ` [Qemu-devel] [PATCH v2 1/1] " Jeff Cody 2015-10-30 17:37 ` Max Reitz 2015-10-30 17:49 ` Jeff Cody
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).