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

[Jeff Cody]

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(-)
>

Test 082 fails now:

  Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2, -o help 
TEST_DIR/t.qcow2
  qemu-img: Invalid option list: backing_file=TEST_DIR/t.qcow2,
 +./common.config: line 117:   737 Segmentation fault      (core dumped) ( exec 
$QEMU_IMG_CMD "$@" )
  
  Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,help 
TEST_DIR/t.qcow2
  qemu-img: Invalid option list: ,help
 +./common.config: line 117:   746 Segmentation fault      (core dumped) ( exec 
$QEMU_IMG_CMD "$@" )
  
  Testing: amend -f qcow2 -o backing_file=TEST_DIR/t.qcow2 -o ,, -o help 
TEST_DIR/t.qcow2
  qemu-img: Invalid option list: ,,
 +./common.config: line 117:   756 Segmentation fault      (core dumped) ( exec 
$QEMU_IMG_CMD "$@" )
  
  Testing: amend -f qcow2 -o help
  Supported options


That shows me your patches are working well :)



> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index 859705f..617f397 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -47,13 +47,6 @@ _supported_os Linux
>  _default_cache_mode "writethrough"
>  _supported_cache_modes "writethrough"
>  
> -_subshell_exec()
> -{
> -    # Executing crashing commands in a subshell prevents information like the
> -    # "Killed" line from being lost
> -    (exec "$@")
> -}
> -
>  size=128M
>  
>  echo
> @@ -74,8 +67,8 @@ echo "== Creating a dirty image file =="
>  IMGOPTS="compat=1.1,lazy_refcounts=on"
>  _make_test_img $size
>  
> -_subshell_exec $QEMU_IO -c "write -P 0x5a 0 512" \
> -                        -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> +$QEMU_IO -c "write -P 0x5a 0 512" \
> +         -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>      | _filter_qemu_io
>  
>  # The dirty bit must be set
> @@ -109,8 +102,8 @@ echo "== Opening a dirty image read/write should repair 
> it =="
>  IMGOPTS="compat=1.1,lazy_refcounts=on"
>  _make_test_img $size
>  
> -_subshell_exec $QEMU_IO -c "write -P 0x5a 0 512" \
> -                        -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> +$QEMU_IO -c "write -P 0x5a 0 512" \
> +         -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>      | _filter_qemu_io
>  
>  # The dirty bit must be set
> @@ -127,8 +120,8 @@ echo "== Creating an image file with lazy_refcounts=off 
> =="
>  IMGOPTS="compat=1.1,lazy_refcounts=off"
>  _make_test_img $size
>  
> -_subshell_exec $QEMU_IO -c "write -P 0x5a 0 512" \
> -                        -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> +$QEMU_IO -c "write -P 0x5a 0 512" \
> +         -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>      | _filter_qemu_io
>  
>  # The dirty bit must not be set since lazy_refcounts=off
> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> index d09751f..613ef1b 100644
> --- a/tests/qemu-iotests/039.out
> +++ b/tests/qemu-iotests/039.out
> @@ -11,7 +11,7 @@ No errors were found on the image.
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>  wrote 512/512 bytes at offset 0
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -./039: Killed                  ( exec "$@" )
> +./common.config: Killed                  ( exec $QEMU_IO_CMD "$@" )
>  incompatible_features     0x1
>  ERROR cluster 5 refcount=0 reference=1
>  ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0
> @@ -46,7 +46,7 @@ read 512/512 bytes at offset 0
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>  wrote 512/512 bytes at offset 0
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -./039: Killed                  ( exec "$@" )
> +./common.config: Killed                  ( exec $QEMU_IO_CMD "$@" )
>  incompatible_features     0x1
>  ERROR cluster 5 refcount=0 reference=1
>  Rebuilding refcount structure
> @@ -60,7 +60,7 @@ incompatible_features     0x0
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
>  wrote 512/512 bytes at offset 0
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -./039: Killed                  ( exec "$@" )
> +./common.config: Killed                  ( exec $QEMU_IO_CMD "$@" )
>  incompatible_features     0x0
>  No errors were found on the image.
>  
> diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
> index 8d37f8a..1df887a 100755
> --- a/tests/qemu-iotests/061
> +++ b/tests/qemu-iotests/061
> @@ -58,7 +58,8 @@ echo
>  echo "=== Testing dirty version downgrade ==="
>  echo
>  IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
> -$QEMU_IO -c "write -P 0x2a 0 128k" -c flush -c abort "$TEST_IMG" | 
> _filter_qemu_io
> +$QEMU_IO -c "write -P 0x2a 0 128k" -c flush -c abort "$TEST_IMG" 2>&1 \
> +    | _filter_qemu_io
>  $PYTHON qcow2.py "$TEST_IMG" dump-header
>  $QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
>  $PYTHON qcow2.py "$TEST_IMG" dump-header
> @@ -91,7 +92,8 @@ echo
>  echo "=== Testing dirty lazy_refcounts=off ==="
>  echo
>  IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
> -$QEMU_IO -c "write -P 0x2a 0 128k" -c flush -c abort "$TEST_IMG" | 
> _filter_qemu_io
> +$QEMU_IO -c "write -P 0x2a 0 128k" -c flush -c abort "$TEST_IMG" 2>&1 \
> +    | _filter_qemu_io
>  $PYTHON qcow2.py "$TEST_IMG" dump-header
>  $QEMU_IMG amend -o "lazy_refcounts=off" "$TEST_IMG"
>  $PYTHON qcow2.py "$TEST_IMG" dump-header
> diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
> index 5ec248f..c9e3917 100644
> --- a/tests/qemu-iotests/061.out
> +++ b/tests/qemu-iotests/061.out
> @@ -57,6 +57,7 @@ No errors were found on the image.
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>  wrote 131072/131072 bytes at offset 0
>  128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +./common.config: Aborted                 (core dumped) ( exec $QEMU_IO_CMD 
> "$@" )
>  magic                     0x514649fb
>  version                   3
>  backing_file_offset       0x0
> @@ -214,6 +215,7 @@ No errors were found on the image.
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>  wrote 131072/131072 bytes at offset 0
>  128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +./common.config: Aborted                 (core dumped) ( exec $QEMU_IO_CMD 
> "$@" )
>  magic                     0x514649fb
>  version                   3
>  backing_file_offset       0x0
> diff --git a/tests/qemu-iotests/check b/tests/qemu-iotests/check
> index 6d58203..b5a535e 100755
> --- a/tests/qemu-iotests/check
> +++ b/tests/qemu-iotests/check
> @@ -231,10 +231,10 @@ FULL_HOST_DETAILS=`_full_platform_details`
>  #FULL_MOUNT_OPTIONS=`_scratch_mount_options`
>  
>  cat <<EOF
> -QEMU          -- $QEMU
> -QEMU_IMG      -- $QEMU_IMG
> -QEMU_IO       -- $QEMU_IO
> -QEMU_NBD      -- $QEMU_NBD
> +QEMU          -- $QEMU_CMD
> +QEMU_IMG      -- $QEMU_IMG_CMD
> +QEMU_IO       -- $QEMU_IO_CMD
> +QEMU_NBD      -- $QEMU_NBD_CMD
>  IMGFMT        -- $FULL_IMGFMT_DETAILS
>  IMGPROTO      -- $FULL_IMGPROTO_DETAILS
>  PLATFORM      -- $FULL_HOST_DETAILS
> diff --git a/tests/qemu-iotests/common.config 
> b/tests/qemu-iotests/common.config
> index e0bf896..480efe6 100644
> --- a/tests/qemu-iotests/common.config
> +++ b/tests/qemu-iotests/common.config
> @@ -103,10 +103,36 @@ if [ -z "$QEMU_NBD_PROG" ]; then
>      export QEMU_NBD_PROG="`set_prog_path qemu-nbd`"
>  fi
>  
> -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" "$@")
+}
+

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