| Message ID | 9e4f958b3895b7259b98d845bb46f000ba362869.1446232490.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 30.10.2015 20:25, 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/058 | 12 ++++++++---- > tests/qemu-iotests/common.config | 14 ++++++++++++-- > tests/qemu-iotests/common.qemu | 18 ++++++++++++------ > tests/qemu-iotests/common.rc | 8 +++++--- > 4 files changed, 37 insertions(+), 15 deletions(-) > > diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058 > index f2bdd0b..63a6598 100755 > --- a/tests/qemu-iotests/058 > +++ b/tests/qemu-iotests/058 > @@ -32,11 +32,17 @@ status=1 # failure is the default! > > nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket > nbd_snapshot_img="nbd:unix:$nbd_unix_socket" > +rm -f "${TEST_DIR}/qemu-nbd.pid" > > _cleanup_nbd() > { > - if [ -n "$NBD_SNAPSHOT_PID" ]; then > - kill "$NBD_SNAPSHOT_PID" > + local NBD_SNAPSHOT_PID > + if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then > + read NBD_SNAPSHOT_PID < "${TEST_DIR}/qemu-nbd.pid" > + rm -f "${TEST_DIR}/qemu-nbd.pid" > + if [ -n "$NBD_SNAPSHOT_PID" ]; then No, I won't complain about using ! -z "" elsewhere and -n "" here. :-) Reviewed-by: Max Reitz <mreitz@redhat.com> > + kill "$NBD_SNAPSHOT_PID" > + fi > fi > rm -f "$nbd_unix_socket" > }
Max Reitz <mreitz@redhat.com> writes: > On 30.10.2015 20:25, 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/058 | 12 ++++++++---- >> tests/qemu-iotests/common.config | 14 ++++++++++++-- >> tests/qemu-iotests/common.qemu | 18 ++++++++++++------ >> tests/qemu-iotests/common.rc | 8 +++++--- >> 4 files changed, 37 insertions(+), 15 deletions(-) >> >> diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058 >> index f2bdd0b..63a6598 100755 >> --- a/tests/qemu-iotests/058 >> +++ b/tests/qemu-iotests/058 >> @@ -32,11 +32,17 @@ status=1 # failure is the default! >> >> nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket >> nbd_snapshot_img="nbd:unix:$nbd_unix_socket" >> +rm -f "${TEST_DIR}/qemu-nbd.pid" >> >> _cleanup_nbd() >> { >> - if [ -n "$NBD_SNAPSHOT_PID" ]; then >> - kill "$NBD_SNAPSHOT_PID" >> + local NBD_SNAPSHOT_PID >> + if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then >> + read NBD_SNAPSHOT_PID < "${TEST_DIR}/qemu-nbd.pid" >> + rm -f "${TEST_DIR}/qemu-nbd.pid" >> + if [ -n "$NBD_SNAPSHOT_PID" ]; then > > No, I won't complain about using ! -z "" elsewhere and -n "" here. :-) The little pedant in me screams "but I will!", and the little prankster next to him is clapping enthusiastically. Kidding aside: not worth a respin, but could be cleaned up on commit (maintainer's discretion). > Reviewed-by: Max Reitz <mreitz@redhat.com> > >> + kill "$NBD_SNAPSHOT_PID" >> + fi >> fi >> rm -f "$nbd_unix_socket" >> }
On 02.11.2015 08:37, Markus Armbruster wrote: > Max Reitz <mreitz@redhat.com> writes: > >> On 30.10.2015 20:25, 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/058 | 12 ++++++++---- >>> tests/qemu-iotests/common.config | 14 ++++++++++++-- >>> tests/qemu-iotests/common.qemu | 18 ++++++++++++------ >>> tests/qemu-iotests/common.rc | 8 +++++--- >>> 4 files changed, 37 insertions(+), 15 deletions(-) >>> >>> diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058 >>> index f2bdd0b..63a6598 100755 >>> --- a/tests/qemu-iotests/058 >>> +++ b/tests/qemu-iotests/058 >>> @@ -32,11 +32,17 @@ status=1 # failure is the default! >>> >>> nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket >>> nbd_snapshot_img="nbd:unix:$nbd_unix_socket" >>> +rm -f "${TEST_DIR}/qemu-nbd.pid" >>> >>> _cleanup_nbd() >>> { >>> - if [ -n "$NBD_SNAPSHOT_PID" ]; then >>> - kill "$NBD_SNAPSHOT_PID" >>> + local NBD_SNAPSHOT_PID >>> + if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then >>> + read NBD_SNAPSHOT_PID < "${TEST_DIR}/qemu-nbd.pid" >>> + rm -f "${TEST_DIR}/qemu-nbd.pid" >>> + if [ -n "$NBD_SNAPSHOT_PID" ]; then >> >> No, I won't complain about using ! -z "" elsewhere and -n "" here. :-) > > The little pedant in me screams "but I will!", and the little prankster > next to him is clapping enthusiastically. > > Kidding aside: not worth a respin, but could be cleaned up on commit > (maintainer's discretion). Oh, if that's the case, then I have another thing for you: The use of == in patch 2! ;-) (I'm a bit disappointed Eric doesn't have a mail filter for #!/bin/(ba)?sh ... if.*== for his mail client.) Max > >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >>> + kill "$NBD_SNAPSHOT_PID" >>> + fi >>> fi >>> rm -f "$nbd_unix_socket" >>> }
On 11/02/2015 09:03 AM, Max Reitz wrote: >>>> +++ b/tests/qemu-iotests/058 >>>> @@ -32,11 +32,17 @@ status=1 # failure is the default! >>>> >>>> nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket >>>> nbd_snapshot_img="nbd:unix:$nbd_unix_socket" >>>> +rm -f "${TEST_DIR}/qemu-nbd.pid" >>>> >>>> _cleanup_nbd() >>>> { >>>> - if [ -n "$NBD_SNAPSHOT_PID" ]; then >>>> - kill "$NBD_SNAPSHOT_PID" >>>> + local NBD_SNAPSHOT_PID >>>> + if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then >>>> + read NBD_SNAPSHOT_PID < "${TEST_DIR}/qemu-nbd.pid" >>>> + rm -f "${TEST_DIR}/qemu-nbd.pid" >>>> + if [ -n "$NBD_SNAPSHOT_PID" ]; then >>> >>> No, I won't complain about using ! -z "" elsewhere and -n "" here. :-) >> >> The little pedant in me screams "but I will!", and the little prankster >> next to him is clapping enthusiastically. >> >> Kidding aside: not worth a respin, but could be cleaned up on commit >> (maintainer's discretion). > > Oh, if that's the case, then I have another thing for you: The use of == > in patch 2! ;-) > > (I'm a bit disappointed Eric doesn't have a mail filter for > #!/bin/(ba)?sh ... if.*== for his mail client.) I already know that most (if not all) of qemu-iotests is specifically /bin/bash. But if we want to, we can ditch -n and -z, and just use: if [[ $NBD_SNAPSHOT_PID ]]; then and similarly. In fact, I actually prefer embracing bash-isms when we know we are using bash, to make it obvious that we know we are not catering to /bin/sh.
diff --git a/tests/qemu-iotests/058 b/tests/qemu-iotests/058 index f2bdd0b..63a6598 100755 --- a/tests/qemu-iotests/058 +++ b/tests/qemu-iotests/058 @@ -32,11 +32,17 @@ status=1 # failure is the default! nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket nbd_snapshot_img="nbd:unix:$nbd_unix_socket" +rm -f "${TEST_DIR}/qemu-nbd.pid" _cleanup_nbd() { - if [ -n "$NBD_SNAPSHOT_PID" ]; then - kill "$NBD_SNAPSHOT_PID" + local NBD_SNAPSHOT_PID + if [ -f "${TEST_DIR}/qemu-nbd.pid" ]; then + read NBD_SNAPSHOT_PID < "${TEST_DIR}/qemu-nbd.pid" + rm -f "${TEST_DIR}/qemu-nbd.pid" + if [ -n "$NBD_SNAPSHOT_PID" ]; then + kill "$NBD_SNAPSHOT_PID" + fi fi rm -f "$nbd_unix_socket" } @@ -60,7 +66,6 @@ _export_nbd_snapshot() { _cleanup_nbd $QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l $1 & - NBD_SNAPSHOT_PID=$! _wait_for_nbd } @@ -68,7 +73,6 @@ _export_nbd_snapshot1() { _cleanup_nbd $QEMU_NBD -v -t -k "$nbd_unix_socket" "$TEST_IMG" -l snapshot.name=$1 & - NBD_SNAPSHOT_PID=$! _wait_for_nbd } 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..5db0611 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,18 @@ 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" + 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 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" ;;
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/058 | 12 ++++++++---- tests/qemu-iotests/common.config | 14 ++++++++++++-- tests/qemu-iotests/common.qemu | 18 ++++++++++++------ tests/qemu-iotests/common.rc | 8 +++++--- 4 files changed, 37 insertions(+), 15 deletions(-)