Re: [Qemu-devel] [PATCH] qemu-iotests: fix -valgrind option for check

[Jeff Cody]

On Fri, Oct 30, 2015 at 06:16:29PM +0100, Max Reitz wrote:
> On 29.10.2015 19:04, Jeff Cody wrote:
> > Commit 934659c switched the iotests to run qemu-io from a bash subshell,
> > in order to catch segfaults.  This method is incompatible with the
> > current valgrind_qemu_io() bash function.
> > 
> > Move the valgrind usage into the exec subshell in _qemu_io_wrapper(),
> > while making sure the original return value is passed back to the
> > caller.
> > 
> > Update test output for tests 039, 061, and 137 as it looks for the
> > specific subshell command when the process is terminated.
> > 
> > Reported-by: Kevin Wolf <address@hidden>
> > Signed-off-by: Jeff Cody <address@hidden>
> > ---
> >  tests/qemu-iotests/039.out       | 30 +++++++++++++++++++++++++-----
> >  tests/qemu-iotests/061.out       | 12 ++++++++++--
> >  tests/qemu-iotests/137.out       |  6 +++++-
> >  tests/qemu-iotests/common        |  9 ++-------
> >  tests/qemu-iotests/common.config | 18 +++++++++++++++++-
> >  tests/qemu-iotests/common.rc     | 10 ----------
> >  6 files changed, 59 insertions(+), 26 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> > index 03a31c5..32c8846 100644
> > --- a/tests/qemu-iotests/039.out
> > +++ b/tests/qemu-iotests/039.out
> > @@ -11,7 +11,11 @@ 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.config: Killed                  ( exec "$QEMU_IO_PROG" 
> > $QEMU_IO_OPTIONS "$@" )
> > +./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" 
> > ]; then
> > +    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> > "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +else
> > +    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +fi )
> >  incompatible_features     0x1
> >  ERROR cluster 5 refcount=0 reference=1
> >  ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0
> > @@ -46,7 +50,11 @@ 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.config: Killed                  ( exec "$QEMU_IO_PROG" 
> > $QEMU_IO_OPTIONS "$@" )
> > +./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" 
> > ]; then
> > +    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> > "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +else
> > +    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +fi )
> >  incompatible_features     0x1
> >  ERROR cluster 5 refcount=0 reference=1
> >  Rebuilding refcount structure
> > @@ -60,7 +68,11 @@ 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.config: Killed                  ( exec "$QEMU_IO_PROG" 
> > $QEMU_IO_OPTIONS "$@" )
> > +./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" 
> > ]; then
> > +    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> > "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +else
> > +    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +fi )
> >  incompatible_features     0x0
> >  No errors were found on the image.
> >  
> > @@ -79,7 +91,11 @@ 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.config: Killed                  ( exec "$QEMU_IO_PROG" 
> > $QEMU_IO_OPTIONS "$@" )
> > +./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" 
> > ]; then
> > +    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> > "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +else
> > +    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +fi )
> >  incompatible_features     0x1
> >  ERROR cluster 5 refcount=0 reference=1
> >  ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0
> > @@ -89,7 +105,11 @@ 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.config: Killed                  ( exec "$QEMU_IO_PROG" 
> > $QEMU_IO_OPTIONS "$@" )
> > +./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" 
> > ]; then
> > +    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> > "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +else
> > +    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +fi )
> >  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 b16bea9..f2598a8 100644
> > --- a/tests/qemu-iotests/061.out
> > +++ b/tests/qemu-iotests/061.out
> > @@ -57,7 +57,11 @@ 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: Killed                  ( exec "$QEMU_IO_PROG" 
> > $QEMU_IO_OPTIONS "$@" )
> > +./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" 
> > ]; then
> > +    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> > "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +else
> > +    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +fi )
> >  magic                     0x514649fb
> >  version                   3
> >  backing_file_offset       0x0
> > @@ -215,7 +219,11 @@ 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: Killed                  ( exec "$QEMU_IO_PROG" 
> > $QEMU_IO_OPTIONS "$@" )
> > +./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" 
> > ]; then
> > +    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> > "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +else
> > +    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +fi )
> >  magic                     0x514649fb
> >  version                   3
> >  backing_file_offset       0x0
> > diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out
> > index cf55a41..88c702c 100644
> > --- a/tests/qemu-iotests/137.out
> > +++ b/tests/qemu-iotests/137.out
> > @@ -31,7 +31,11 @@ Cache clean interval too big
> >  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.config: Killed                  ( exec "$QEMU_IO_PROG" 
> > $QEMU_IO_OPTIONS "$@" )
> > +./common.config: Killed                  ( if [ "${VALGRIND_QEMU}" == "y" 
> > ]; then
> > +    exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 
> > "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +else
> > +    exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@";
> > +fi )
> >  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 b/tests/qemu-iotests/common
> > index 25c351b..ff84f4b 100644
> > --- a/tests/qemu-iotests/common
> > +++ b/tests/qemu-iotests/common
> > @@ -41,7 +41,6 @@ sortme=false
> >  expunge=true
> >  have_test_arg=false
> >  randomize=false
> > -valgrind=false
> >  cachemode=false
> >  rm -f $tmp.list $tmp.tmp $tmp.sed
> >  
> > @@ -53,6 +52,7 @@ export CACHEMODE="writeback"
> >  export QEMU_IO_OPTIONS=""
> >  export CACHEMODE_IS_DEFAULT=true
> >  export QEMU_OPTIONS="-nodefaults"
> > +export VALGRIND_QEMU=
> >  
> >  for r
> >  do
> > @@ -278,7 +278,7 @@ testlist options
> >              ;;
> >  
> >          -valgrind)
> > -            valgrind=true
> > +            VALGRIND_QEMU='y'
> >              xpand=false
> >              ;;
> >  
> > @@ -436,8 +436,3 @@ fi
> >  if [ "$IMGPROTO" = "nbd" ] ; then
> >      [ "$QEMU_NBD" = "" ] && _fatal "qemu-nbd not found"
> >  fi
> > -
> > -if $valgrind; then
> > -    export REAL_QEMU_IO="$QEMU_IO_PROG"
> > -    export QEMU_IO_PROG=valgrind_qemu_io
> > -fi
> > diff --git a/tests/qemu-iotests/common.config 
> > b/tests/qemu-iotests/common.config
> > index 4d8665f..db9702b 100644
> > --- a/tests/qemu-iotests/common.config
> > +++ b/tests/qemu-iotests/common.config
> > @@ -122,7 +122,23 @@ _qemu_img_wrapper()
> >  
> >  _qemu_io_wrapper()
> >  {
> > -    (exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@")
> > +    local VALGRIND_LOGFILE=/tmp/$$.valgrind
> > +    local RETVAL
> > +    (
> > +        if [ "${VALGRIND_QEMU}" == "y" ]; then
> > +            exec valgrind --log-file="${VALGRIND_LOGFILE}" 
> > --error-exitcode=99 "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@"
> > +        else
> > +            exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@"
> > +        fi
> > +    )
> > +    RETVAL=$?
> 
> Er, well, this is nice... When just invoking $QEMU_IO -c 'sigraise 9', I
> get the appropriate error message. ("$PID Killed [...]"). But the
> instant an image is opened, it just disappears. Yes, using -c open makes
> it disappear, too.
> 
> Since all of our qemu-io invocations do use image files (that's its
> purpose after all), that means that all of them seem to exit just fine
> when running under valgrind. That is... strange.
> 
> Is it just me? Maybe I have a broken valgrind, I don't know (3.11.0 here).
>

I have valgrind-3.9.0 here, and I get the same behavior... it is not
just you.  There are also some tests where valgrind itself segfaults.

I was going to suggest using kill -l TERM instead of kill -l KILL.
However, I just tried that with test 137, and it causes valgrind to
segfault. :(

> > +    if [ "${VALGRIND_QEMU}" == "y" ]; then
> > +        if [ $RETVAL != 0 ]; then
> > +            cat "${VALGRIND_LOGFILE}"
> 
> If I got the error message and RETVAL would be correctly set to 137,
> this would print the log file. I'm not sure whether that's what we
> want...? If valgrind exits with any error code but 99, the log file will
> probably not contain anything interesting.
> 
> But if the qemu-io process was killed on purpose, this breaks the test,
> which I don't think is necessary.
>

Good point... How about just:

 if [ $RETVAL == 99 ]; then
      cat "${VALGRIND_LOGFILE}"
 fi


> > +        fi
> > +        rm -f "${VALGRIND_LOGFILE}"
> > +    fi
> > +    (exit $RETVAL)
> >  }
> >  
> >  _qemu_nbd_wrapper()
> > diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> > index 4878e99..d9913f8 100644
> > --- a/tests/qemu-iotests/common.rc
> > +++ b/tests/qemu-iotests/common.rc
> > @@ -70,16 +70,6 @@ else
> >      TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
> >  fi
> >  
> > -function valgrind_qemu_io()
> > -{
> > -    valgrind --log-file=/tmp/$$.valgrind --error-exitcode=99 $REAL_QEMU_IO 
> > "$@"
> > -    if [ $? != 0 ]; then
> > -        cat /tmp/$$.valgrind
> > -    fi
> > -    rm -f /tmp/$$.valgrind
> > -}
> > -
> > -
> >  _optstr_add()
> >  {
> >      if [ -n "$1" ]; then
> > 
> 
>