Re: [Qemu-block] [PATCH 1/2] iotests: Do not suppress segfaults in bash tests

[Jeff Cody]

On Wed, Sep 02, 2015 at 05:09:41PM +0200, Max Reitz wrote:
> On 01.09.2015 00:55, Jeff Cody wrote:
> > On Mon, Aug 31, 2015 at 09:05:12PM +0200, Max Reitz wrote:
> >> Currently, if a qemu/qemu-io/qemu-img/qemu-nbd invocation receives a
> >> segmentation fault, that message is invisible in most cases since the
> >> output is generally filtered and bash suppresses the segmentation fault
> >> notice for any but the last element of a pipe.
> >>
> >> Most of the time, the test will then fail anyway because of missing
> >> output, but not necessarily (as happened with test 82 recently).
> >>
> >> Fix this by making the corresponding environment variables point to
> >> wrapper functions which execute the respective command in a subshell.
> >>
> >> Signed-off-by: Max Reitz <address@hidden>
> >> ---
> >>  tests/qemu-iotests/039           | 19 ++++++-------------
> >>  tests/qemu-iotests/039.out       |  6 +++---
> >>  tests/qemu-iotests/061           |  6 ++++--
> >>  tests/qemu-iotests/061.out       |  2 ++
> >>  tests/qemu-iotests/check         |  8 ++++----
> >>  tests/qemu-iotests/common.config | 34 ++++++++++++++++++++++++++++++----
> >>  tests/qemu-iotests/common.rc     | 12 +++++++++++-
> >>  tests/qemu-iotests/iotests.py    |  6 +++---
> >>  8 files changed, 63 insertions(+), 30 deletions(-)
> >>

[snip]

> >>  
> >> -export QEMU="$QEMU_PROG $QEMU_OPTIONS"
> >> -export QEMU_IMG=$QEMU_IMG_PROG
> >> -export QEMU_IO="$QEMU_IO_PROG $QEMU_IO_OPTIONS"
> >> -export QEMU_NBD=$QEMU_NBD_PROG
> >> +export QEMU_CMD="$QEMU_PROG $QEMU_OPTIONS"
> >> +export QEMU_IMG_CMD=$QEMU_IMG_PROG
> >> +export QEMU_IO_CMD="$QEMU_IO_PROG $QEMU_IO_OPTIONS"
> >> +export QEMU_NBD_CMD=$QEMU_NBD_PROG
> >> +
> > 
> > Unfortunately, these exports (old and new) make it so that pathnames
> > with spaces won't work (in case someone hasn't had it beaten into them
> > that spaced pathnames is begging for trouble...).  But luckily, I
> > think your patch make it easier to fix this, since you have the
> > wrapper!
> > 
> > I think we want to drop the _OPTIONS in each of the exports, e.g.:
> > 
> > -export QEMU="$QEMU_PROG $QEMU_OPTIONS"
> > +export QEMU_CMD="$QEMU_PROG"
> > 
> > And then instead of this:
> > 
> >> +_qemu_wrapper()
> >> +{
> >> +    (exec $QEMU_CMD "$@")
> >> +}
> >> +
> > 
> > Use this form, instead:
> > 
> > +_qemu_wrapper()
> > +{
> > +    (exec "$QEMU_CMD" "$QEMU_OPTIONS" "$@")
> > +}
> > +
> 
> Yes, I tried not to break anything in that regard that wasn't already
> broken, but if we have the chance to fix something, then we (I) should
> go for it.
> 
> QEMU_CMD is used for the Python tests as the command line to be used for
> qemu invocation, so it cannot be modified like that. But what I can do
> is to drop QEMU.*_CMD and replace it by "$QEMU.*_PROG" "$QEMU.*_OPTIONS"
> everywhere, I guess. I'll have a look.
>

Good point.

I don't think it needs to be done in this patch series, as your
patches don't change this behavior.  It could be done in a follow-up
series, by you or someone else.

I'm happy to give my r-b as-is, and we can change it later.:

Reviewed-by: Jeff Cody <address@hidden>

> Thanks for reviewing!
> 
> Max

You're welcome :)

> 
> >> +_qemu_img_wrapper()
> >> +{
> >> +    (exec $QEMU_IMG_CMD "$@")
> >> +}
> >> +
> >> +_qemu_io_wrapper()
> >> +{
> >> +    (exec $QEMU_IO_CMD "$@")
> >> +}
> >> +
> >> +_qemu_nbd_wrapper()
> >> +{
> >> +    (exec $QEMU_NBD_CMD "$@")
> >> +}
> >> +
> > 
> > Repeat as appropriate, above.
> > 
> >> +export QEMU=_qemu_wrapper
> >> +export QEMU_IMG=_qemu_img_wrapper
> >> +export QEMU_IO=_qemu_io_wrapper
> >> +export QEMU_NBD=_qemu_nbd_wrapper
> >> +
> >>  default_machine=$($QEMU -machine \? | awk '/(default)/{print $1}')
> >>  default_alias_machine=$($QEMU -machine \? |\
> >>      awk -v var_default_machine="$default_machine"\)\
> >> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> >> index 22d3514..28e4bea 100644
> >> --- a/tests/qemu-iotests/common.rc
> >> +++ b/tests/qemu-iotests/common.rc
> >> @@ -439,7 +439,17 @@ _unsupported_imgopts()
> >>  #
> >>  _require_command()
> >>  {
> >> -    eval c=\$$1
> >> +    if [ "$1" = "QEMU" ]; then
> >> +        c=$QEMU_PROG
> >> +    elif [ "$1" = "QEMU_IMG" ]; then
> >> +        c=$QEMU_IMG_PROG
> >> +    elif [ "$1" = "QEMU_IO" ]; then
> >> +        c=$QEMU_IO_PROG
> >> +    elif [ "$1" = "QEMU_NBD" ]; then
> >> +        c=$QEMU_NBD_PROG
> >> +    else
> >> +        eval c=\$$1
> >> +    fi
> >>      [ -x "$c" ] || _notrun "$1 utility required, skipped this test"
> >>  }
> >>  
> >> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
> >> index 5579253..927c74a 100644
> >> --- a/tests/qemu-iotests/iotests.py
> >> +++ b/tests/qemu-iotests/iotests.py
> >> @@ -33,9 +33,9 @@ __all__ = ['imgfmt', 'imgproto', 'test_dir' 'qemu_img', 
> >> 'qemu_io',
> >>  
> >>  # This will not work if arguments or path contain spaces but is necessary 
> >> if we
> >>  # want to support the override options that ./check supports.
> >> -qemu_img_args = os.environ.get('QEMU_IMG', 'qemu-img').strip().split(' ')
> >> -qemu_io_args = os.environ.get('QEMU_IO', 'qemu-io').strip().split(' ')
> >> -qemu_args = os.environ.get('QEMU', 'qemu').strip().split(' ')
> >> +qemu_img_args = os.environ.get('QEMU_IMG_CMD', 
> >> 'qemu-img').strip().split(' ')
> >> +qemu_io_args = os.environ.get('QEMU_IO_CMD', 'qemu-io').strip().split(' ')
> >> +qemu_args = os.environ.get('QEMU_CMD', 'qemu').strip().split(' ')
> >>  
> >>  imgfmt = os.environ.get('IMGFMT', 'raw')
> >>  imgproto = os.environ.get('IMGPROTO', 'file')
> >> -- 
> >> 2.5.0
> >>
> >>
> 
>