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 <mre...@redhat.com>
> >> ---
> >> 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 <jc...@redhat.com>
> 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
> >>
> >>
>
>
