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 <js...@redhat.com>
> > Signed-off-by: Jeff Cody <jc...@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.
