From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:36414)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <jcody@redhat.com>) id 1ZrlHA-0000VT-2h
	for qemu-devel@nongnu.org; Thu, 29 Oct 2015 07:21:49 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <jcody@redhat.com>) id 1ZrlH9-0001tB-9h
	for qemu-devel@nongnu.org; Thu, 29 Oct 2015 07:21:48 -0400
Date: Thu, 29 Oct 2015 07:21:37 -0400
From: Jeff Cody <jcody@redhat.com>
Message-ID: <20151029112137.GA2628@localhost.localdomain>
References: <1aebb2401ecdeff9a85f9f19c981caacf0ed69d3.1446080991.git.jcody@redhat.com>
	<56319BCA.7000708@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <56319BCA.7000708@redhat.com>
Subject: Re: [Qemu-devel] [PATCH] qemu-iotests: fix cleanup of background
 processes
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Eric Blake <eblake@redhat.com>
Cc: kwolf@redhat.com, jsnow@redhat.com, qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@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.