Re: [Qemu-devel] [PATCH v2 1/1] qemu-iotests: fix cleanup of background processes

[Jeff Cody <jcody@redhat.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 <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