Re: [Qemu-devel] [PATCH v6 1/6] iotests: allow Valgrind checking all QEMU processes

[John Snow <jsnow@redhat.com>]

On 8/26/19 11:50 AM, Andrey Shinkevich wrote:
> With the '-valgrind' option, let all the QEMU processes be run under
> the Valgrind tool. The Valgrind own parameters may be set with its
> environment variable VALGRIND_OPTS, e.g.
> $ VALGRIND_OPTS="--leak-check=yes" ./check -valgrind <test#>
> or they may be listed in the Valgrind checked file ./.valgrindrc or
> ~/.valgrindrc like
> --memcheck:leak-check=no
> --memcheck:track-origins=yes
> To exclude a specific process from running under the Valgrind, the
> corresponding environment variable VALGRIND_QEMU_<name> is to be unset:
> $ VALGRIND_QEMU_IO= ./check -valgrind <test#>
> When QEMU-IO process is being killed, the shell report refers to the
> text of the command in _qemu_io_wrapper(), which was modified with this
> patch. So, the benchmark output for the tests 039, 061 and 137 is to be
> changed also.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>  tests/qemu-iotests/039.out   |  30 ++----------
>  tests/qemu-iotests/061.out   |  12 +----
>  tests/qemu-iotests/137.out   |   6 +--
>  tests/qemu-iotests/common.rc | 107 +++++++++++++++++++++++++++++++++++--------
>  4 files changed, 97 insertions(+), 58 deletions(-)
> 
> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> index 724d7b2..66d2159 100644
> --- a/tests/qemu-iotests/039.out
> +++ b/tests/qemu-iotests/039.out
> @@ -11,11 +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)
> -./common.rc: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -    exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed                  ( VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>  incompatible_features     0x1
>  ERROR cluster 5 refcount=0 reference=1
>  ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0
> @@ -50,11 +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)
> -./common.rc: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -    exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed                  ( VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>  incompatible_features     0x1
>  ERROR cluster 5 refcount=0 reference=1
>  Rebuilding refcount structure
> @@ -68,11 +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)
> -./common.rc: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -    exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed                  ( VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>  incompatible_features     0x0
>  No errors were found on the image.
>  
> @@ -91,11 +79,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)
> -./common.rc: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -    exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed                  ( VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>  incompatible_features     0x1
>  ERROR cluster 5 refcount=0 reference=1
>  ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0
> @@ -105,11 +89,7 @@ Data may be corrupted, or further writes to the image may corrupt it.
>  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)
> -./common.rc: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -    exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed                  ( VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>  incompatible_features     0x0
>  No errors were found on the image.
>  *** done
> diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
> index 1aa7d37..346e654 100644
> --- a/tests/qemu-iotests/061.out
> +++ b/tests/qemu-iotests/061.out
> @@ -118,11 +118,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.rc: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -    exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed                  ( VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>  magic                     0x514649fb
>  version                   3
>  backing_file_offset       0x0
> @@ -280,11 +276,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.rc: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -    exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed                  ( VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>  magic                     0x514649fb
>  version                   3
>  backing_file_offset       0x0
> diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out
> index 22d59df..225e6f6 100644
> --- a/tests/qemu-iotests/137.out
> +++ b/tests/qemu-iotests/137.out
> @@ -35,11 +35,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>  qemu-io: Unsupported value 'blubb' for qcow2 option 'overlap-check'. Allowed are any of the following: none, constant, cached, all
>  wrote 512/512 bytes at offset 0
>  512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -./common.rc: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" ]; then
> -    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -else
> -    exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@";
> -fi )
> +./common.rc: Killed                  ( VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>  incompatible_features     0x0
>  Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
>  wrote 65536/65536 bytes at offset 0
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 5502c3d..289686b 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -60,61 +60,132 @@ if ! . ./common.config
>      exit 1
>  fi
>  
> +# Unset the variables to turn Valgrind off for specific processes, e.g.
> +# $ VALGRIND_QEMU_IO= ./check -qcow2 -valgrind 015
> +
> +: ${VALGRIND_QEMU_VM='y'}
> +: ${VALGRIND_QEMU_IMG='y'}
> +: ${VALGRIND_QEMU_IO='y'}
> +: ${VALGRIND_QEMU_NBD='y'}
> +: ${VALGRIND_QEMU_VXHS='y'}
> +

I have to admit to you that I'm not familiar with this trick. I'm
looking it up and I see := documented, but not = alone.

It doesn't seem documented here at all:
https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html

I see it here, though:
https://www.tldp.org/LDP/abs/html/parameter-substitution.html

And it seems to work, but I'm not sure if this works with BSD or OSX's
sh. I see Eric comment on that compatibility a lot, so maybe I'll let
him chime in.

The other option, if this is not portable, is to have a NO_VALGRIND_XYZ
variable that can be set by the user and checked instead without needing
to set a default.


> +# The Valgrind own parameters may be set with
> +# its environment variable VALGRIND_OPTS, e.g.
> +# $ VALGRIND_OPTS="--leak-check=yes" ./check -qcow2 -valgrind 015
> +
> +_qemu_proc_exec()
> +{
> +    local VALGRIND_LOGFILE="$1"
> +    shift
> +    if [ "${VALGRIND_QEMU}" == "y" ]; then
> +        exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$@"
> +    else
> +        exec "$@"
> +    fi
> +}
> +
> +_qemu_proc_valgrind_log()
> +{
> +    local VALGRIND_LOGFILE="$1"
> +    local RETVAL="$2"
> +    if [ "${VALGRIND_QEMU}" == "y" ]; then
> +        if [ $RETVAL == 99 ]; then
> +            cat "${VALGRIND_LOGFILE}"
> +        fi
> +        rm -f "${VALGRIND_LOGFILE}"
> +    fi
> +}
> +
>  _qemu_wrapper()
>  {
> +    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
> +    local VALGRIND_ON="${VALGRIND_QEMU}"
> +    if [ "${VALGRIND_QEMU_VM}" != "y" ]; then
> +        VALGRIND_ON=''
> +    fi
>      (
>          if [ -n "${QEMU_NEED_PID}" ]; then
>              echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
>          fi
> -        exec "$QEMU_PROG" $QEMU_OPTIONS "$@"
> +        VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" \
> +            "$QEMU_PROG" $QEMU_OPTIONS "$@"
>      )
> +    RETVAL=$?
> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
> +    return $RETVAL
>  }
>  
>  _qemu_img_wrapper()
>  {
> -    (exec "$QEMU_IMG_PROG" $QEMU_IMG_OPTIONS "$@")
> +    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
> +    local VALGRIND_ON="${VALGRIND_QEMU}"
> +    if [ "${VALGRIND_QEMU_IMG}" != "y" ]; then
> +        VALGRIND_ON=''
> +    fi
> +    (
> +        VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" \
> +            "$QEMU_IMG_PROG" $QEMU_IMG_OPTIONS "$@"
> +    )
> +    RETVAL=$?
> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
> +    return $RETVAL
>  }
>  
>  _qemu_io_wrapper()
>  {
>      local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
>      local QEMU_IO_ARGS="$QEMU_IO_OPTIONS"
> +    local VALGRIND_ON="${VALGRIND_QEMU}"
> +    if [ "${VALGRIND_QEMU_IO}" != "y" ]; then
> +        VALGRIND_ON=''
> +    fi
>      if [ "$IMGOPTSSYNTAX" = "true" ]; then
>          QEMU_IO_ARGS="--image-opts $QEMU_IO_ARGS"
>          if [ -n "$IMGKEYSECRET" ]; then
>              QEMU_IO_ARGS="--object secret,id=keysec0,data=$IMGKEYSECRET $QEMU_IO_ARGS"
>          fi
>      fi
> -    local RETVAL
>      (
> -        if [ "${VALGRIND_QEMU}" == "y" ]; then
> -            exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@"
> -        else
> -            exec "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@"
> -        fi
> +        VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" \
> +            "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@"
>      )
>      RETVAL=$?
> -    if [ "${VALGRIND_QEMU}" == "y" ]; then
> -        if [ $RETVAL == 99 ]; then
> -            cat "${VALGRIND_LOGFILE}"
> -        fi
> -        rm -f "${VALGRIND_LOGFILE}"
> -    fi
> -    (exit $RETVAL)
> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
> +    return $RETVAL
>  }
>  
>  _qemu_nbd_wrapper()
>  {
> -    "$QEMU_NBD_PROG" --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" \
> -                     $QEMU_NBD_OPTIONS "$@"
> +    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
> +    local VALGRIND_ON="${VALGRIND_QEMU}"
> +    if [ "${VALGRIND_QEMU_NBD}" != "y" ]; then
> +        VALGRIND_ON=''
> +    fi
> +    (
> +        VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" \
> +            "$QEMU_NBD_PROG" --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" \
> +             $QEMU_NBD_OPTIONS "$@"
> +    )
> +    RETVAL=$?
> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
> +    return $RETVAL
>  }
>  
>  _qemu_vxhs_wrapper()
>  {
> +    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
> +    local VALGRIND_ON="${VALGRIND_QEMU}"
> +    if [ "${VALGRIND_QEMU_VXHS}" != "y" ]; then
> +        VALGRIND_ON=''
> +    fi
>      (
>          echo $BASHPID > "${TEST_DIR}/qemu-vxhs.pid"
> -        exec "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
> +        VALGRIND_QEMU="${VALGRIND_ON}" _qemu_proc_exec "${VALGRIND_LOGFILE}" \
> +            "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
>      )
> +    RETVAL=$?
> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
> +    return $RETVAL
>  }
>  
>  export QEMU=_qemu_wrapper
> 

-- 
—js