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

[Jeff Cody <jcody@redhat.com>]

On Wed, Oct 28, 2015 at 10:08:42PM -0600, Eric Blake wrote:
> 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"
>

Yes, that would be better.

> > +            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.
> 

Good catch, thanks.