diff mbox series

[v6,1/6] iotests: allow Valgrind checking all QEMU processes

Message ID 1566834628-485525-2-git-send-email-andrey.shinkevich@virtuozzo.com
State New
Headers show
Series Allow Valgrind checking all QEMU processes | expand

Commit Message

Andrey Shinkevich Aug. 26, 2019, 3:50 p.m. UTC 
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(-)

Comments

John Snow Aug. 28, 2019, 10:58 p.m. UTC | #1 
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
>
Eric Blake Aug. 29, 2019, 12:30 a.m. UTC | #2 
On 8/28/19 5:58 PM, John Snow wrote:

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

That's not unsetting, that's setting to the empty string.

>> +# $ 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's been a repeated complaint to the bash developer that the manual is
doing a disservice to its users by not documenting ${var=val} in an
easily searchable form.  It IS documented, but only by virtue of
${var:=val} occurring under a section header that states:

       When not performing substring expansion,  using  the  forms
documented
       below  (e.g.,  :-),  bash  tests for a parameter that is unset or
null.
       Omitting the colon results in a test  only  for  a  parameter
that  is
       unset.

So the choice is whether you want to special case a variable set to an
empty string the same as an unset variable, or the same as a variable
with a non-empty value.

> 
> 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.

It's quite portable; POSIX requires it, and autoconf relies on it.
Andrey Shinkevich Aug. 29, 2019, 10:50 a.m. UTC | #3 
On 29/08/2019 03:30, Eric Blake wrote:
> On 8/28/19 5:58 PM, John Snow wrote:
> 
>>> +++ 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.
> 
> That's not unsetting, that's setting to the empty string.
> 

Thanks Eric, I will make the correction of the comment. Any string other 
than "y", including the empty one, fits.

>>> +# $ 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 am going to make the change:

: ${VALGRIND_QEMU_VM=$VALGRIND_QEMU}
: ${VALGRIND_QEMU_IMG=$VALGRIND_QEMU}
: ${VALGRIND_QEMU_IO=$VALGRIND_QEMU}
: ${VALGRIND_QEMU_NBD=$VALGRIND_QEMU}
: ${VALGRIND_QEMU_VXHS=$VALGRIND_QEMU}

and get rid of the local VALGRIND_ON="${VALGRIND_QEMU}"

so that the code will be optimized.

>> 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's been a repeated complaint to the bash developer that the manual is
> doing a disservice to its users by not documenting ${var=val} in an
> easily searchable form.  It IS documented, but only by virtue of
> ${var:=val} occurring under a section header that states:
> 
>         When not performing substring expansion,  using  the  forms
> documented
>         below  (e.g.,  :-),  bash  tests for a parameter that is unset or
> null.
>         Omitting the colon results in a test  only  for  a  parameter
> that  is
>         unset.
> 
> So the choice is whether you want to special case a variable set to an
> empty string the same as an unset variable, or the same as a variable
> with a non-empty value.
> 

Thank you all for your reviews and comments. The purpose why I omitted 
the colon is to allow a user writing the shorter command syntax like
$ VALGRIND_QEMU_IO= ./check -valgrind <test#>
rather than
$ VALGRIND_QEMU_IO=" 'no' or 'off' or else anything other than 'y' " 
./check -valgrind <test#>
so, no need to strike the Shift key twice and guess at what else is 
acceptable to type )))

The variable default value 'y' looks good to me to implement the new 
functionality that is compatible with the existing one when we just set 
the '-valgrind' switch. The general idea behind using the Valgrind is to 
make a careful search for memory issues. Once found, a user can tune the 
particular test with extra variables to save their development/testing 
time as John suggested. Also, no need to specify all the five long name 
variables each time a user writes the command if default values aren't set.

I am flexible to make a change that is good for all. So, what solution 
will we come to?

Andrey

>>
>> 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.
> 
> It's quite portable; POSIX requires it, and autoconf relies on it.
>
John Snow Aug. 29, 2019, 5:33 p.m. UTC | #4 
On 8/29/19 6:50 AM, Andrey Shinkevich wrote:
> 
> 
> On 29/08/2019 03:30, Eric Blake wrote:
>> On 8/28/19 5:58 PM, John Snow wrote:
>>
>>>> +++ 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.
>>
>> That's not unsetting, that's setting to the empty string.
>>
> 
> Thanks Eric, I will make the correction of the comment. Any string other 
> than "y", including the empty one, fits.
> 
>>>> +# $ 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 am going to make the change:
> 
> : ${VALGRIND_QEMU_VM=$VALGRIND_QEMU}
> : ${VALGRIND_QEMU_IMG=$VALGRIND_QEMU}
> : ${VALGRIND_QEMU_IO=$VALGRIND_QEMU}
> : ${VALGRIND_QEMU_NBD=$VALGRIND_QEMU}
> : ${VALGRIND_QEMU_VXHS=$VALGRIND_QEMU}
> 
> and get rid of the local VALGRIND_ON="${VALGRIND_QEMU}"
> 
> so that the code will be optimized.
> 

Seems good!

>>> 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's been a repeated complaint to the bash developer that the manual is
>> doing a disservice to its users by not documenting ${var=val} in an
>> easily searchable form.  It IS documented, but only by virtue of
>> ${var:=val} occurring under a section header that states:
>>
>>         When not performing substring expansion,  using  the  forms
>> documented
>>         below  (e.g.,  :-),  bash  tests for a parameter that is unset or
>> null.
>>         Omitting the colon results in a test  only  for  a  parameter
>> that  is
>>         unset.
>>
>> So the choice is whether you want to special case a variable set to an
>> empty string the same as an unset variable, or the same as a variable
>> with a non-empty value.
>>
> 
> Thank you all for your reviews and comments. The purpose why I omitted 
> the colon is to allow a user writing the shorter command syntax like
> $ VALGRIND_QEMU_IO= ./check -valgrind <test#>
> rather than
> $ VALGRIND_QEMU_IO=" 'no' or 'off' or else anything other than 'y' " 
> ./check -valgrind <test#>
> so, no need to strike the Shift key twice and guess at what else is 
> acceptable to type )))
> 
> The variable default value 'y' looks good to me to implement the new 
> functionality that is compatible with the existing one when we just set 
> the '-valgrind' switch. The general idea behind using the Valgrind is to 
> make a careful search for memory issues. Once found, a user can tune the 
> particular test with extra variables to save their development/testing 
> time as John suggested. Also, no need to specify all the five long name 
> variables each time a user writes the command if default values aren't set.
> 
> I am flexible to make a change that is good for all. So, what solution 
> will we come to?
> 

I don't actually really have a preference here; it's development and
testing infrastructure. As long as it is POSIX portable, I'm happy. If
we goof it up, we'll find out eventually. If we don't, well. Just more
evidence we need more non-Linux contributors.

--js
diff mbox series

Patch

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'}
+
+# 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