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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2945 bytes --]

On 10/28/2015 07:15 PM, 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     |  6 +++---
>  3 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
> index 596bb2b..5fd4ca8 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 -n $BASHPID > "${TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"

'echo -n' is a non-portable bashism; even in bash, it can be made to
behave differently by 'set -o posix; shopt -s xpg_echo'.  It's safer,
and shorter, to use 'printf', if you don't need the newline.

On the other hand, if you use plain 'echo', and include the newline,...

> @@ -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
> +            QEMU_PID=`cat "${TEST_DIR}/qemu-${i}.pid"`

...then you could avoid the subshell and useless use of cat here by doing:

read QEMU_PID < "${TEST_DIR}/qemu-${i}.pid"

> +            rm -f "${TEST_DIR}/qemu-${i}.pid"
> +        fi
> +        if [ -z "${wait}" ] && [ ! -z ${QEMU_PID} ]; then

Missing quotes around ${QEMU_PID}.  But you got lucky: if it is empty,
then you are evaluating [ ! -z ], which is false; where the intended [ !
-z "" ] would also be false.  Still, it's bad form to abuse [] like that.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]