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

[Max Reitz <mreitz@redhat.com>]

[-- 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 --]