[Qemu-devel] [PATCH v2 0/7] Allow Valgrind checking all QEMU processes

[Qemu-devel] [PATCH v2 0/7] Allow Valgrind checking all QEMU processes

[Andrey Shinkevich]

In the current implementation of the QEMU bash iotests, only qemu-io
processes may be run under the Valgrind, which is a useful tool for
finding memory usage issues. Let's allow the common.rc bash script
runing all the QEMU processes, such as qemu-kvm, qemu-img, qemu-ndb
and qemu-vxhs, under the Valgrind tool.

v2:
  01: The patch 2/7 of v1 was merged into the patch 1/7, suggested by Daniel.
  02: Another patch 7/7 was added to introduce the Valgrind error suppression
      file into the QEMU project.

Andrey Shinkevich (7):
  iotests: allow Valgrind checking all QEMU processes
  iotests: exclude killed processes from running under Valgrind
  iotests: Valgrind fails to work with nonexistent directory
  iotests: extended timeout under Valgrind
  iotests: extend sleeping time under Valgrind
  iotests: amend QEMU NBD process synchronization
  iotests: new file to suppress Valgrind errors

 tests/qemu-iotests/028           |  6 +++-
 tests/qemu-iotests/039           |  5 +++
 tests/qemu-iotests/039.out       | 30 +++---------------
 tests/qemu-iotests/051           |  1 +
 tests/qemu-iotests/061           |  2 ++
 tests/qemu-iotests/061.out       | 12 ++-----
 tests/qemu-iotests/137           |  1 +
 tests/qemu-iotests/137.out       |  6 +---
 tests/qemu-iotests/183           |  9 +++++-
 tests/qemu-iotests/192           |  6 +++-
 tests/qemu-iotests/247           |  6 +++-
 tests/qemu-iotests/common.nbd    |  6 ++++
 tests/qemu-iotests/common.rc     | 68 ++++++++++++++++++++++++++++++----------
 tests/qemu-iotests/valgrind.supp | 31 ++++++++++++++++++
 14 files changed, 128 insertions(+), 61 deletions(-)
 create mode 100644 tests/qemu-iotests/valgrind.supp

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/7] iotests: allow Valgrind checking all QEMU processes

[Andrey Shinkevich]

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 -qcow2 -valgrind <test#>
or they may be listed in the Valgrind checked file ./.valgrindrc or
~/.valgrindrc like
--memcheck:leak-check=no
--memcheck:track-origins=yes
After including the Valgrind into the QEMU processes wrappers in the
common.rc script, the benchmark output for the tests 039 061 137 is to
be amended.

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 | 65 ++++++++++++++++++++++++++++++++------------
 4 files changed, 56 insertions(+), 57 deletions(-)

diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
index 724d7b2..972c6c0 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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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..8cb57eb 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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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..7fed5e6 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                  ( _qemu_proc_wrapper "${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 93f8738..3caaca4 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -60,19 +60,52 @@ if ! . ./common.config
     exit 1
 fi
 
+_qemu_proc_wrapper()
+{
+    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
     (
         if [ -n "${QEMU_NEED_PID}" ]; then
             echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
         fi
-        exec "$QEMU_PROG" $QEMU_OPTIONS "$@"
+        _qemu_proc_wrapper "${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
+    (
+        _qemu_proc_wrapper "${VALGRIND_LOGFILE}" "$QEMU_IMG_PROG" $QEMU_IMG_OPTIONS "$@"
+    )
+    RETVAL=$?
+    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
+    return $RETVAL
 }
 
 _qemu_io_wrapper()
@@ -85,38 +118,36 @@ _qemu_io_wrapper()
             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
+        _qemu_proc_wrapper "${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()
 {
+    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
     (
         echo $BASHPID > "${QEMU_TEST_DIR}/qemu-nbd.pid"
-        exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
+        _qemu_proc_wrapper "${VALGRIND_LOGFILE}" "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
     )
+    RETVAL=$?
+    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
+    return $RETVAL
 }
 
 _qemu_vxhs_wrapper()
 {
+    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
     (
         echo $BASHPID > "${TEST_DIR}/qemu-vxhs.pid"
-        exec "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
+        _qemu_proc_wrapper "${VALGRIND_LOGFILE}" "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
     )
+    RETVAL=$?
+    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
+    return $RETVAL
 }
 
 export QEMU=_qemu_wrapper
-- 
1.8.3.1

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

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, 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 -qcow2 -valgrind <test#>
> or they may be listed in the Valgrind checked file ./.valgrindrc or
> ~/.valgrindrc like
> --memcheck:leak-check=no
> --memcheck:track-origins=yes
> After including the Valgrind into the QEMU processes wrappers in the
> common.rc script, the benchmark output for the tests 039 061 137 is to
> be amended.
> 
> 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 | 65 ++++++++++++++++++++++++++++++++------------
>   4 files changed, 56 insertions(+), 57 deletions(-)
> 
> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> index 724d7b2..972c6c0 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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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..8cb57eb 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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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..7fed5e6 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                  ( _qemu_proc_wrapper "${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 93f8738..3caaca4 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -60,19 +60,52 @@ if ! . ./common.config
>       exit 1
>   fi
>   
> +_qemu_proc_wrapper()
> +{
> +    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
>       (
>           if [ -n "${QEMU_NEED_PID}" ]; then
>               echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
>           fi
> -        exec "$QEMU_PROG" $QEMU_OPTIONS "$@"
> +        _qemu_proc_wrapper "${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
> +    (
> +        _qemu_proc_wrapper "${VALGRIND_LOGFILE}" "$QEMU_IMG_PROG" $QEMU_IMG_OPTIONS "$@"
> +    )
> +    RETVAL=$?
> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
> +    return $RETVAL


this usage of _qemu_proc_wrapper and _qemu_proc_valgrind_log are almost identical in all
new _qemu* wrappers. Could you create one _qemu_valgrind_wrapper, to not duplicate this code?


>   }
>   
>   _qemu_io_wrapper()
> @@ -85,38 +118,36 @@ _qemu_io_wrapper()
>               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
> +        _qemu_proc_wrapper "${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()
>   {
> +    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
>       (
>           echo $BASHPID > "${QEMU_TEST_DIR}/qemu-nbd.pid"
> -        exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
> +        _qemu_proc_wrapper "${VALGRIND_LOGFILE}" "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
>       )
> +    RETVAL=$?
> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
> +    return $RETVAL
>   }
>   
>   _qemu_vxhs_wrapper()
>   {
> +    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
>       (
>           echo $BASHPID > "${TEST_DIR}/qemu-vxhs.pid"
> -        exec "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
> +        _qemu_proc_wrapper "${VALGRIND_LOGFILE}" "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
>       )
> +    RETVAL=$?
> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
> +    return $RETVAL
>   }
>   
>   export QEMU=_qemu_wrapper
> 


-- 
Best regards,
Vladimir

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

[Andrey Shinkevich]

On 13/06/2019 12:44, Vladimir Sementsov-Ogievskiy wrote:
> 11.06.2019 21:02, 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 -qcow2 -valgrind <test#>
>> or they may be listed in the Valgrind checked file ./.valgrindrc or
>> ~/.valgrindrc like
>> --memcheck:leak-check=no
>> --memcheck:track-origins=yes
>> After including the Valgrind into the QEMU processes wrappers in the
>> common.rc script, the benchmark output for the tests 039 061 137 is to
>> be amended.
>>
>> 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 | 65 ++++++++++++++++++++++++++++++++------------
>>    4 files changed, 56 insertions(+), 57 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
>> index 724d7b2..972c6c0 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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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..8cb57eb 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                  ( _qemu_proc_wrapper "${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                  ( _qemu_proc_wrapper "${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..7fed5e6 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                  ( _qemu_proc_wrapper "${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 93f8738..3caaca4 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -60,19 +60,52 @@ if ! . ./common.config
>>        exit 1
>>    fi
>>    
>> +_qemu_proc_wrapper()
>> +{
>> +    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
>>        (
>>            if [ -n "${QEMU_NEED_PID}" ]; then
>>                echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid"
>>            fi
>> -        exec "$QEMU_PROG" $QEMU_OPTIONS "$@"
>> +        _qemu_proc_wrapper "${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
>> +    (
>> +        _qemu_proc_wrapper "${VALGRIND_LOGFILE}" "$QEMU_IMG_PROG" $QEMU_IMG_OPTIONS "$@"
>> +    )
>> +    RETVAL=$?
>> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
>> +    return $RETVAL
> 
> 
> this usage of _qemu_proc_wrapper and _qemu_proc_valgrind_log are almost identical in all
> new _qemu* wrappers. Could you create one _qemu_valgrind_wrapper, to not duplicate this code?
> 
> 

I agree that the wrappers look like of the same pattern at a first 
glance. However, they have different conditions inside. The list of 
arguments differs also. Moreover, _qemu_proc_wrapper() and 
_qemu_proc_valgrind_log() are run in different shells. That makes 
implementation of a unified wrapper complicated.

>>    }
>>    
>>    _qemu_io_wrapper()
>> @@ -85,38 +118,36 @@ _qemu_io_wrapper()
>>                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
>> +        _qemu_proc_wrapper "${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()
>>    {
>> +    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
>>        (
>>            echo $BASHPID > "${QEMU_TEST_DIR}/qemu-nbd.pid"
>> -        exec "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
>> +        _qemu_proc_wrapper "${VALGRIND_LOGFILE}" "$QEMU_NBD_PROG" $QEMU_NBD_OPTIONS "$@"
>>        )
>> +    RETVAL=$?
>> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
>> +    return $RETVAL
>>    }
>>    
>>    _qemu_vxhs_wrapper()
>>    {
>> +    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
>>        (
>>            echo $BASHPID > "${TEST_DIR}/qemu-vxhs.pid"
>> -        exec "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
>> +        _qemu_proc_wrapper "${VALGRIND_LOGFILE}" "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
>>        )
>> +    RETVAL=$?
>> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
>> +    return $RETVAL
>>    }
>>    
>>    export QEMU=_qemu_wrapper
>>
> 
> 

-- 
With the best regards,
Andrey Shinkevich

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

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, 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 -qcow2 -valgrind <test#>
> or they may be listed in the Valgrind checked file ./.valgrindrc or
> ~/.valgrindrc like
> --memcheck:leak-check=no
> --memcheck:track-origins=yes
> After including the Valgrind into the QEMU processes wrappers in the
> common.rc script, the benchmark output for the tests 039 061 137 is to
> be amended.

If tests outputs changed, please describe here in short: why?


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v2 2/7] iotests: exclude killed processes from running under Valgrind

[Andrey Shinkevich]

The Valgrind tool fails to manage its termination when QEMU raises the
signal SIGKILL. Lets exclude such test cases from running under the
Valgrind because there is no sense to check memory issues that way.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/039 | 5 +++++
 tests/qemu-iotests/061 | 2 ++
 tests/qemu-iotests/137 | 1 +
 3 files changed, 8 insertions(+)

diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 0d4e963..95115e2 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -65,6 +65,7 @@ echo "== Creating a dirty image file =="
 IMGOPTS="compat=1.1,lazy_refcounts=on"
 _make_test_img $size
 
+VALGRIND_QEMU="" \
 $QEMU_IO -c "write -P 0x5a 0 512" \
          -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
     | _filter_qemu_io
@@ -100,6 +101,7 @@ echo "== Opening a dirty image read/write should repair it =="
 IMGOPTS="compat=1.1,lazy_refcounts=on"
 _make_test_img $size
 
+VALGRIND_QEMU="" \
 $QEMU_IO -c "write -P 0x5a 0 512" \
          -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
     | _filter_qemu_io
@@ -118,6 +120,7 @@ echo "== Creating an image file with lazy_refcounts=off =="
 IMGOPTS="compat=1.1,lazy_refcounts=off"
 _make_test_img $size
 
+VALGRIND_QEMU="" \
 $QEMU_IO -c "write -P 0x5a 0 512" \
          -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
     | _filter_qemu_io
@@ -151,6 +154,7 @@ echo "== Changing lazy_refcounts setting at runtime =="
 IMGOPTS="compat=1.1,lazy_refcounts=off"
 _make_test_img $size
 
+VALGRIND_QEMU="" \
 $QEMU_IO -c "reopen -o lazy-refcounts=on" \
          -c "write -P 0x5a 0 512" \
          -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
@@ -163,6 +167,7 @@ _check_test_img
 IMGOPTS="compat=1.1,lazy_refcounts=on"
 _make_test_img $size
 
+VALGRIND_QEMU="" \
 $QEMU_IO -c "reopen -o lazy-refcounts=off" \
          -c "write -P 0x5a 0 512" \
          -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index d7dbd7e..5d0724c 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -73,6 +73,7 @@ echo
 echo "=== Testing dirty version downgrade ==="
 echo
 IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
+VALGRIND_QEMU="" \
 $QEMU_IO -c "write -P 0x2a 0 128k" -c flush \
          -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 | _filter_qemu_io
 $PYTHON qcow2.py "$TEST_IMG" dump-header
@@ -107,6 +108,7 @@ echo
 echo "=== Testing dirty lazy_refcounts=off ==="
 echo
 IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
+VALGRIND_QEMU="" \
 $QEMU_IO -c "write -P 0x2a 0 128k" -c flush \
          -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 | _filter_qemu_io
 $PYTHON qcow2.py "$TEST_IMG" dump-header
diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
index 0c3d2a1..a442fc8 100755
--- a/tests/qemu-iotests/137
+++ b/tests/qemu-iotests/137
@@ -130,6 +130,7 @@ echo
 
 # Whether lazy-refcounts was actually enabled can easily be tested: Check if
 # the dirty bit is set after a crash
+VALGRIND_QEMU="" \
 $QEMU_IO \
     -c "reopen -o lazy-refcounts=on,overlap-check=blubb" \
     -c "write -P 0x5a 0 512" \
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 2/7] iotests: exclude killed processes from running under Valgrind

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> The Valgrind tool fails to manage its termination when QEMU raises the
> signal SIGKILL. Lets exclude such test cases from running under the
> Valgrind because there is no sense to check memory issues that way.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/039 | 5 +++++
>   tests/qemu-iotests/061 | 2 ++
>   tests/qemu-iotests/137 | 1 +
>   3 files changed, 8 insertions(+)
> 
> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index 0d4e963..95115e2 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -65,6 +65,7 @@ echo "== Creating a dirty image file =="
>   IMGOPTS="compat=1.1,lazy_refcounts=on"
>   _make_test_img $size
>   
> +VALGRIND_QEMU="" \


Shouldn't it be written once per test, just without "\" ?

>   $QEMU_IO -c "write -P 0x5a 0 512" \
>            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>       | _filter_qemu_io
> @@ -100,6 +101,7 @@ echo "== Opening a dirty image read/write should repair it =="
>   IMGOPTS="compat=1.1,lazy_refcounts=on"
>   _make_test_img $size
>   
> +VALGRIND_QEMU="" \
>   $QEMU_IO -c "write -P 0x5a 0 512" \
>            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>       | _filter_qemu_io
> @@ -118,6 +120,7 @@ echo "== Creating an image file with lazy_refcounts=off =="
>   IMGOPTS="compat=1.1,lazy_refcounts=off"
>   _make_test_img $size
>   
> +VALGRIND_QEMU="" \
>   $QEMU_IO -c "write -P 0x5a 0 512" \
>            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>       | _filter_qemu_io
> @@ -151,6 +154,7 @@ echo "== Changing lazy_refcounts setting at runtime =="
>   IMGOPTS="compat=1.1,lazy_refcounts=off"
>   _make_test_img $size
>   
> +VALGRIND_QEMU="" \
>   $QEMU_IO -c "reopen -o lazy-refcounts=on" \
>            -c "write -P 0x5a 0 512" \
>            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> @@ -163,6 +167,7 @@ _check_test_img
>   IMGOPTS="compat=1.1,lazy_refcounts=on"
>   _make_test_img $size
>   
> +VALGRIND_QEMU="" \
>   $QEMU_IO -c "reopen -o lazy-refcounts=off" \
>            -c "write -P 0x5a 0 512" \
>            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
> index d7dbd7e..5d0724c 100755
> --- a/tests/qemu-iotests/061
> +++ b/tests/qemu-iotests/061
> @@ -73,6 +73,7 @@ echo
>   echo "=== Testing dirty version downgrade ==="
>   echo
>   IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
> +VALGRIND_QEMU="" \
>   $QEMU_IO -c "write -P 0x2a 0 128k" -c flush \
>            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 | _filter_qemu_io
>   $PYTHON qcow2.py "$TEST_IMG" dump-header
> @@ -107,6 +108,7 @@ echo
>   echo "=== Testing dirty lazy_refcounts=off ==="
>   echo
>   IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
> +VALGRIND_QEMU="" \
>   $QEMU_IO -c "write -P 0x2a 0 128k" -c flush \
>            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 | _filter_qemu_io
>   $PYTHON qcow2.py "$TEST_IMG" dump-header
> diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
> index 0c3d2a1..a442fc8 100755
> --- a/tests/qemu-iotests/137
> +++ b/tests/qemu-iotests/137
> @@ -130,6 +130,7 @@ echo
>   
>   # Whether lazy-refcounts was actually enabled can easily be tested: Check if
>   # the dirty bit is set after a crash
> +VALGRIND_QEMU="" \
>   $QEMU_IO \
>       -c "reopen -o lazy-refcounts=on,overlap-check=blubb" \
>       -c "write -P 0x5a 0 512" \
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 2/7] iotests: exclude killed processes from running under Valgrind

[Roman Kagan]

On Thu, Jun 13, 2019 at 12:47:21PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 11.06.2019 21:02, Andrey Shinkevich wrote:
> > The Valgrind tool fails to manage its termination when QEMU raises the
> > signal SIGKILL. Lets exclude such test cases from running under the
> > Valgrind because there is no sense to check memory issues that way.
> > 
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > ---
> >   tests/qemu-iotests/039 | 5 +++++
> >   tests/qemu-iotests/061 | 2 ++
> >   tests/qemu-iotests/137 | 1 +
> >   3 files changed, 8 insertions(+)
> > 
> > diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> > index 0d4e963..95115e2 100755
> > --- a/tests/qemu-iotests/039
> > +++ b/tests/qemu-iotests/039
> > @@ -65,6 +65,7 @@ echo "== Creating a dirty image file =="
> >   IMGOPTS="compat=1.1,lazy_refcounts=on"
> >   _make_test_img $size
> >   
> > +VALGRIND_QEMU="" \
> 
> 
> Shouldn't it be written once per test, just without "\" ?

Only qemu-io invocations that perform raise(KILL) need to bypass
valgrinding.  Clearing VALGRIND_QEMU globally will indulge all qemu-io
throughout the test.

Roman.

> >   $QEMU_IO -c "write -P 0x5a 0 512" \
> >            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> >       | _filter_qemu_io
> > @@ -100,6 +101,7 @@ echo "== Opening a dirty image read/write should repair it =="
> >   IMGOPTS="compat=1.1,lazy_refcounts=on"
> >   _make_test_img $size
> >   
> > +VALGRIND_QEMU="" \
> >   $QEMU_IO -c "write -P 0x5a 0 512" \
> >            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> >       | _filter_qemu_io
> > @@ -118,6 +120,7 @@ echo "== Creating an image file with lazy_refcounts=off =="
> >   IMGOPTS="compat=1.1,lazy_refcounts=off"
> >   _make_test_img $size
> >   
> > +VALGRIND_QEMU="" \
> >   $QEMU_IO -c "write -P 0x5a 0 512" \
> >            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> >       | _filter_qemu_io
> > @@ -151,6 +154,7 @@ echo "== Changing lazy_refcounts setting at runtime =="
> >   IMGOPTS="compat=1.1,lazy_refcounts=off"
> >   _make_test_img $size
> >   
> > +VALGRIND_QEMU="" \
> >   $QEMU_IO -c "reopen -o lazy-refcounts=on" \
> >            -c "write -P 0x5a 0 512" \
> >            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> > @@ -163,6 +167,7 @@ _check_test_img
> >   IMGOPTS="compat=1.1,lazy_refcounts=on"
> >   _make_test_img $size
> >   
> > +VALGRIND_QEMU="" \
> >   $QEMU_IO -c "reopen -o lazy-refcounts=off" \
> >            -c "write -P 0x5a 0 512" \
> >            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> > diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
> > index d7dbd7e..5d0724c 100755
> > --- a/tests/qemu-iotests/061
> > +++ b/tests/qemu-iotests/061
> > @@ -73,6 +73,7 @@ echo
> >   echo "=== Testing dirty version downgrade ==="
> >   echo
> >   IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
> > +VALGRIND_QEMU="" \
> >   $QEMU_IO -c "write -P 0x2a 0 128k" -c flush \
> >            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 | _filter_qemu_io
> >   $PYTHON qcow2.py "$TEST_IMG" dump-header
> > @@ -107,6 +108,7 @@ echo
> >   echo "=== Testing dirty lazy_refcounts=off ==="
> >   echo
> >   IMGOPTS="compat=1.1,lazy_refcounts=on" _make_test_img 64M
> > +VALGRIND_QEMU="" \
> >   $QEMU_IO -c "write -P 0x2a 0 128k" -c flush \
> >            -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 | _filter_qemu_io
> >   $PYTHON qcow2.py "$TEST_IMG" dump-header
> > diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
> > index 0c3d2a1..a442fc8 100755
> > --- a/tests/qemu-iotests/137
> > +++ b/tests/qemu-iotests/137
> > @@ -130,6 +130,7 @@ echo
> >   
> >   # Whether lazy-refcounts was actually enabled can easily be tested: Check if
> >   # the dirty bit is set after a crash
> > +VALGRIND_QEMU="" \
> >   $QEMU_IO \
> >       -c "reopen -o lazy-refcounts=on,overlap-check=blubb" \
> >       -c "write -P 0x5a 0 512" \
> > 
> 
> 
> -- 
> Best regards,
> Vladimir

Re: [Qemu-devel] [PATCH v2 2/7] iotests: exclude killed processes from running under Valgrind

[Kevin Wolf]

Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben:
> The Valgrind tool fails to manage its termination when QEMU raises the
> signal SIGKILL. Lets exclude such test cases from running under the
> Valgrind because there is no sense to check memory issues that way.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

I don't fully understand the reasoning here. Most interesting memory
access errors happen before a process terminates. (I'm not talking about
leaks here, but use-after-free, buffer overflows, uninitialised memory
etc.)

However, I do see that running these test cases with -valgrind ends in a
hang because the valgrind process keeps hanging around as a zombie
process and the test case doesn't reap it. I'm not exactly sure why that
is, but it looks more like a problem with the parent process (i.e. the
bash script).

If we can't figure out how to fix this, we can disable valgrind in these
cases, but I think the explanation needs to be different.

> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index 0d4e963..95115e2 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -65,6 +65,7 @@ echo "== Creating a dirty image file =="
>  IMGOPTS="compat=1.1,lazy_refcounts=on"
>  _make_test_img $size
>  
> +VALGRIND_QEMU="" \
>  $QEMU_IO -c "write -P 0x5a 0 512" \
>           -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
>      | _filter_qemu_io

I agree with Vladimir that setting VALGRIND_QEMU only once at the top of
the script is probably the better option.

Kevin

Re: [Qemu-devel] [PATCH v2 2/7] iotests: exclude killed processes from running under Valgrind

[Roman Kagan]

On Mon, Jun 17, 2019 at 01:15:04PM +0200, Kevin Wolf wrote:
> Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben:
> > The Valgrind tool fails to manage its termination when QEMU raises the
> > signal SIGKILL. Lets exclude such test cases from running under the
> > Valgrind because there is no sense to check memory issues that way.
> > 
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> 
> I don't fully understand the reasoning here. Most interesting memory
> access errors happen before a process terminates. (I'm not talking about
> leaks here, but use-after-free, buffer overflows, uninitialised memory
> etc.)

Nothing of the above, and nothing in general, happens in the usermode
process upon SIGKILL delivery.  

> However, I do see that running these test cases with -valgrind ends in a
> hang because the valgrind process keeps hanging around as a zombie
> process and the test case doesn't reap it. I'm not exactly sure why that
> is, but it looks more like a problem with the parent process (i.e. the
> bash script).

It rather looks like valgrind getting confused about what to do with
raise(SIGKILL) in the multithreaded case.

> If we can't figure out how to fix this, we can disable valgrind in these
> cases, but I think the explanation needs to be different.
> 
> > diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> > index 0d4e963..95115e2 100755
> > --- a/tests/qemu-iotests/039
> > +++ b/tests/qemu-iotests/039
> > @@ -65,6 +65,7 @@ echo "== Creating a dirty image file =="
> >  IMGOPTS="compat=1.1,lazy_refcounts=on"
> >  _make_test_img $size
> >  
> > +VALGRIND_QEMU="" \
> >  $QEMU_IO -c "write -P 0x5a 0 512" \
> >           -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> >      | _filter_qemu_io
> 
> I agree with Vladimir that setting VALGRIND_QEMU only once at the top of
> the script is probably the better option.

It is not, because there's no reason for qemu-io invocations that don't
perform raise(SIGKILL) to escape valgrinding.

Roman.

Re: [Qemu-devel] [PATCH v2 2/7] iotests: exclude killed processes from running under Valgrind

[Kevin Wolf]

Am 17.06.2019 um 14:18 hat Roman Kagan geschrieben:
> On Mon, Jun 17, 2019 at 01:15:04PM +0200, Kevin Wolf wrote:
> > Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben:
> > > The Valgrind tool fails to manage its termination when QEMU raises the
> > > signal SIGKILL. Lets exclude such test cases from running under the
> > > Valgrind because there is no sense to check memory issues that way.
> > > 
> > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > 
> > I don't fully understand the reasoning here. Most interesting memory
> > access errors happen before a process terminates. (I'm not talking about
> > leaks here, but use-after-free, buffer overflows, uninitialised memory
> > etc.)
> 
> Nothing of the above, and nothing in general, happens in the usermode
> process upon SIGKILL delivery.

My point is, the interesting part is what the program does before
SIGKILL happens. There is value in reporting memory errors as long as we
can, even if the final check doesn't happen because of SIGKILL.

> > However, I do see that running these test cases with -valgrind ends in a
> > hang because the valgrind process keeps hanging around as a zombie
> > process and the test case doesn't reap it. I'm not exactly sure why that
> > is, but it looks more like a problem with the parent process (i.e. the
> > bash script).
> 
> It rather looks like valgrind getting confused about what to do with
> raise(SIGKILL) in the multithreaded case.

Well, valgrind can't do anything with SIGKILL, obviously, because it's
killed immediately. But maybe the kernel does get confused for some
reason. I get the main threads as a zombie, but a second is still
running. Sending SIGKILL to the second thread, too, makes the test case
complete successfully.

So I guess the main question is why the second thread isn't
automatically killed when the main thread receives SIGKILL.

Kevin

Re: [Qemu-devel] [PATCH v2 2/7] iotests: exclude killed processes from running under Valgrind

[Roman Kagan]

On Mon, Jun 17, 2019 at 02:53:55PM +0200, Kevin Wolf wrote:
> Am 17.06.2019 um 14:18 hat Roman Kagan geschrieben:
> > On Mon, Jun 17, 2019 at 01:15:04PM +0200, Kevin Wolf wrote:
> > > Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben:
> > > > The Valgrind tool fails to manage its termination when QEMU raises the
> > > > signal SIGKILL. Lets exclude such test cases from running under the
> > > > Valgrind because there is no sense to check memory issues that way.
> > > > 
> > > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > > 
> > > I don't fully understand the reasoning here. Most interesting memory
> > > access errors happen before a process terminates. (I'm not talking about
> > > leaks here, but use-after-free, buffer overflows, uninitialised memory
> > > etc.)
> > 
> > Nothing of the above, and nothing in general, happens in the usermode
> > process upon SIGKILL delivery.
> 
> My point is, the interesting part is what the program does before
> SIGKILL happens. There is value in reporting memory errors as long as we
> can, even if the final check doesn't happen because of SIGKILL.

Agreed in general, but here the testcases that include 'sigraise 9' only
do simple operations before that which are covered elsewhere too.  So
the extra effort on making valgrind work with these testcases arguably
isn't worth the extra value to be gained.

> > > However, I do see that running these test cases with -valgrind ends in a
> > > hang because the valgrind process keeps hanging around as a zombie
> > > process and the test case doesn't reap it. I'm not exactly sure why that
> > > is, but it looks more like a problem with the parent process (i.e. the
> > > bash script).
> > 
> > It rather looks like valgrind getting confused about what to do with
> > raise(SIGKILL) in the multithreaded case.
> 
> Well, valgrind can't do anything with SIGKILL, obviously, because it's
> killed immediately.

Right, but it can do whatever it wants with raise(SIGKILL).  I haven't
looked at valgrind sources, but

  # strace -ff valgind qemu-io -c 'sigraise 9'

shows SIGKILL neither sent nor received by any thread; it just shows the
main thread exit and the second thread getting stuck waiting on a futex.

> But maybe the kernel does get confused for some
> reason. I get the main threads as a zombie, but a second is still
> running. Sending SIGKILL to the second thread, too, makes the test case
> complete successfully.
> 
> So I guess the main question is why the second thread isn't
> automatically killed when the main thread receives SIGKILL.

I don't see any thread receive SIGKILL.  So I tend to think this is
valgrind's bug/feature.

Anyway the problem is outside of QEMU, so I think we need to weigh the
costs of investigating it and implementing a workaround with the
potential benefit.

Roman.

Re: [Qemu-devel] [PATCH v2 2/7] iotests: exclude killed processes from running under Valgrind

[Kevin Wolf]

Am 17.06.2019 um 15:20 hat Roman Kagan geschrieben:
> On Mon, Jun 17, 2019 at 02:53:55PM +0200, Kevin Wolf wrote:
> > Am 17.06.2019 um 14:18 hat Roman Kagan geschrieben:
> > > On Mon, Jun 17, 2019 at 01:15:04PM +0200, Kevin Wolf wrote:
> > > > Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben:
> > > > > The Valgrind tool fails to manage its termination when QEMU raises the
> > > > > signal SIGKILL. Lets exclude such test cases from running under the
> > > > > Valgrind because there is no sense to check memory issues that way.
> > > > > 
> > > > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > > > 
> > > > I don't fully understand the reasoning here. Most interesting memory
> > > > access errors happen before a process terminates. (I'm not talking about
> > > > leaks here, but use-after-free, buffer overflows, uninitialised memory
> > > > etc.)
> > > 
> > > Nothing of the above, and nothing in general, happens in the usermode
> > > process upon SIGKILL delivery.
> > 
> > My point is, the interesting part is what the program does before
> > SIGKILL happens. There is value in reporting memory errors as long as we
> > can, even if the final check doesn't happen because of SIGKILL.
> 
> Agreed in general, but here the testcases that include 'sigraise 9' only
> do simple operations before that which are covered elsewhere too.  So
> the extra effort on making valgrind work with these testcases arguably
> isn't worth the extra value to be gained.

Ok, fair enough.

> > > > However, I do see that running these test cases with -valgrind ends in a
> > > > hang because the valgrind process keeps hanging around as a zombie
> > > > process and the test case doesn't reap it. I'm not exactly sure why that
> > > > is, but it looks more like a problem with the parent process (i.e. the
> > > > bash script).
> > > 
> > > It rather looks like valgrind getting confused about what to do with
> > > raise(SIGKILL) in the multithreaded case.
> > 
> > Well, valgrind can't do anything with SIGKILL, obviously, because it's
> > killed immediately.
> 
> Right, but it can do whatever it wants with raise(SIGKILL).  I haven't
> looked at valgrind sources, but
> 
>   # strace -ff valgind qemu-io -c 'sigraise 9'
> 
> shows SIGKILL neither sent nor received by any thread; it just shows the
> main thread exit and the second thread getting stuck waiting on a futex.

Oh, I didn't see this! So there isn't even a real SIGKILL signal.

> > But maybe the kernel does get confused for some
> > reason. I get the main threads as a zombie, but a second is still
> > running. Sending SIGKILL to the second thread, too, makes the test case
> > complete successfully.
> > 
> > So I guess the main question is why the second thread isn't
> > automatically killed when the main thread receives SIGKILL.
> 
> I don't see any thread receive SIGKILL.  So I tend to think this is
> valgrind's bug/feature.
> 
> Anyway the problem is outside of QEMU, so I think we need to weigh the
> costs of investigating it and implementing a workaround with the
> potential benefit.

I'd suggest to file a bug against valgrind at least. And indeed just
disable valgrind here like this patch does.

Kevin

Re: [Qemu-devel] [PATCH v2 2/7] iotests: exclude killed processes from running under Valgrind

[Andrey Shinkevich]

On 17/06/2019 17:51, Kevin Wolf wrote:
> Am 17.06.2019 um 15:20 hat Roman Kagan geschrieben:
>> On Mon, Jun 17, 2019 at 02:53:55PM +0200, Kevin Wolf wrote:
>>> Am 17.06.2019 um 14:18 hat Roman Kagan geschrieben:
>>>> On Mon, Jun 17, 2019 at 01:15:04PM +0200, Kevin Wolf wrote:
>>>>> Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben:
>>>>>> The Valgrind tool fails to manage its termination when QEMU raises the
>>>>>> signal SIGKILL. Lets exclude such test cases from running under the
>>>>>> Valgrind because there is no sense to check memory issues that way.
>>>>>>
>>>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>>>
>>>>> I don't fully understand the reasoning here. Most interesting memory
>>>>> access errors happen before a process terminates. (I'm not talking about
>>>>> leaks here, but use-after-free, buffer overflows, uninitialised memory
>>>>> etc.)
>>>>
>>>> Nothing of the above, and nothing in general, happens in the usermode
>>>> process upon SIGKILL delivery.
>>>
>>> My point is, the interesting part is what the program does before
>>> SIGKILL happens. There is value in reporting memory errors as long as we
>>> can, even if the final check doesn't happen because of SIGKILL.
>>
>> Agreed in general, but here the testcases that include 'sigraise 9' only
>> do simple operations before that which are covered elsewhere too.  So
>> the extra effort on making valgrind work with these testcases arguably
>> isn't worth the extra value to be gained.
> 
> Ok, fair enough.
> 
>>>>> However, I do see that running these test cases with -valgrind ends in a
>>>>> hang because the valgrind process keeps hanging around as a zombie
>>>>> process and the test case doesn't reap it. I'm not exactly sure why that
>>>>> is, but it looks more like a problem with the parent process (i.e. the
>>>>> bash script).
>>>>
>>>> It rather looks like valgrind getting confused about what to do with
>>>> raise(SIGKILL) in the multithreaded case.
>>>
>>> Well, valgrind can't do anything with SIGKILL, obviously, because it's
>>> killed immediately.
>>
>> Right, but it can do whatever it wants with raise(SIGKILL).  I haven't
>> looked at valgrind sources, but
>>
>>    # strace -ff valgind qemu-io -c 'sigraise 9'
>>
>> shows SIGKILL neither sent nor received by any thread; it just shows the
>> main thread exit and the second thread getting stuck waiting on a futex.
> 
> Oh, I didn't see this! So there isn't even a real SIGKILL signal.
> 
>>> But maybe the kernel does get confused for some
>>> reason. I get the main threads as a zombie, but a second is still
>>> running. Sending SIGKILL to the second thread, too, makes the test case
>>> complete successfully.
>>>
>>> So I guess the main question is why the second thread isn't
>>> automatically killed when the main thread receives SIGKILL.
>>
>> I don't see any thread receive SIGKILL.  So I tend to think this is
>> valgrind's bug/feature.
>>
>> Anyway the problem is outside of QEMU, so I think we need to weigh the
>> costs of investigating it and implementing a workaround with the
>> potential benefit.
> 
> I'd suggest to file a bug against valgrind at least. And indeed just
> disable valgrind here like this patch does.
> 
> Kevin
> 

I have reported the issue to the KDE Bugtracking System on bugs.kde.org
as instructed on the www.valgrind.org/support/bug_reports.html

The bug 409141 "Valgrind hangs when SIGKILLed" has been created.
The thread can be seen on https://bugs.kde.org/show_bug.cgi?id=409141

Andrey

[Qemu-devel] [PATCH v2 3/7] iotests: Valgrind fails to work with nonexistent directory

[Andrey Shinkevich]

The Valgrind uses the exported variable TMPDIR and fails if the
directory does not exist. Let us exclude such a test case from
being run under the Valgrind.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/051 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 200660f..ccc5bc2 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -377,6 +377,7 @@ printf %b "qemu-io $device_id \"write -P 0x33 0 4k\"\ncommit $device_id\n" |
 $QEMU_IO -c "read -P 0x33 0 4k" "$TEST_IMG" | _filter_qemu_io
 
 # Using snapshot=on with a non-existent TMPDIR
+VALGRIND_QEMU="" \
 TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on
 
 # Using snapshot=on together with read-only=on
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 3/7] iotests: Valgrind fails to work with nonexistent directory

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> The Valgrind uses the exported variable TMPDIR and fails if the
> directory does not exist. Let us exclude such a test case from
> being run under the Valgrind.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

Hmm, isn't it preferable to skip unsupported by
valgrind iotests, instead silently disable valgrind in them?

> ---
>   tests/qemu-iotests/051 | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> index 200660f..ccc5bc2 100755
> --- a/tests/qemu-iotests/051
> +++ b/tests/qemu-iotests/051
> @@ -377,6 +377,7 @@ printf %b "qemu-io $device_id \"write -P 0x33 0 4k\"\ncommit $device_id\n" |
>   $QEMU_IO -c "read -P 0x33 0 4k" "$TEST_IMG" | _filter_qemu_io
>   
>   # Using snapshot=on with a non-existent TMPDIR

(you can add into comment: "Valgrind needs TMPDIR, so disable it"

> +VALGRIND_QEMU="" \
>   TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on
>   
>   # Using snapshot=on together with read-only=on
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 3/7] iotests: Valgrind fails to work with nonexistent directory

[Kevin Wolf]

Am 13.06.2019 um 11:52 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 11.06.2019 21:02, Andrey Shinkevich wrote:
> > The Valgrind uses the exported variable TMPDIR and fails if the
> > directory does not exist. Let us exclude such a test case from
> > being run under the Valgrind.
> > 
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> 
> Hmm, isn't it preferable to skip unsupported by
> valgrind iotests, instead silently disable valgrind in them?

You mean like this at the start of the script?

[ "$VALGRIND_QEMU" == "y" ] && _notrun "valgrind needs a working TMPDIR"

I agree that silently doing something different that the user requested
is bad and visibly skipping the test is often better. On the other hand,
051 is relatively long and it's only one subtest that doesn't work with
valgrind.

Hm... How about we do run the test, but put a note in the casenotrun
file so it's visible that we didn't use valgrind for everything?

Kevin

[Qemu-devel] [PATCH v2 4/7] iotests: extended timeout under Valgrind

[Andrey Shinkevich]

As the iotests run longer under the Valgrind, the QEMU_COMM_TIMEOUT is
to be increased in the test cases 028, 183 and 192 when running under
the Valgrind.

Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/028 | 6 +++++-
 tests/qemu-iotests/183 | 9 ++++++++-
 tests/qemu-iotests/192 | 6 +++++-
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/028 b/tests/qemu-iotests/028
index 01f4959..2fd4405 100755
--- a/tests/qemu-iotests/028
+++ b/tests/qemu-iotests/028
@@ -110,7 +110,11 @@ echo
 qemu_comm_method="monitor"
 _launch_qemu -drive file="${TEST_IMG}",cache=${CACHEMODE},id=disk
 h=$QEMU_HANDLE
-QEMU_COMM_TIMEOUT=1
+if [ "${VALGRIND_QEMU}" == "y" ]; then
+    QEMU_COMM_TIMEOUT=4
+else
+    QEMU_COMM_TIMEOUT=1
+fi
 
 # Silence output since it contains the disk image path and QEMU's readline
 # character echoing makes it very hard to filter the output. Plus, there
diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183
index fbe5a99..71feab8 100755
--- a/tests/qemu-iotests/183
+++ b/tests/qemu-iotests/183
@@ -94,8 +94,15 @@ if echo "$reply" | grep "compiled without old-style" > /dev/null; then
     _notrun "migrate -b support not compiled in"
 fi
 
-QEMU_COMM_TIMEOUT=0.1 qemu_cmd_repeat=50 silent=yes \
+timeout_comm=$QEMU_COMM_TIMEOUT
+if [ "${VALGRIND_QEMU}" == "y" ]; then
+    QEMU_COMM_TIMEOUT=1
+else
+    QEMU_COMM_TIMEOUT=0.1
+fi
+qemu_cmd_repeat=50 silent=yes \
     _send_qemu_cmd $src "{ 'execute': 'query-migrate' }" '"status": "completed"'
+QEMU_COMM_TIMEOUT=$timeout_comm
 _send_qemu_cmd $src "{ 'execute': 'query-status' }" "return"
 
 echo
diff --git a/tests/qemu-iotests/192 b/tests/qemu-iotests/192
index 6193257..11ea037 100755
--- a/tests/qemu-iotests/192
+++ b/tests/qemu-iotests/192
@@ -60,7 +60,11 @@ fi
 qemu_comm_method="monitor"
 _launch_qemu -drive $DRIVE_ARG -incoming defer
 h=$QEMU_HANDLE
-QEMU_COMM_TIMEOUT=1
+if [ "${VALGRIND_QEMU}" == "y" ]; then
+    QEMU_COMM_TIMEOUT=4
+else
+    QEMU_COMM_TIMEOUT=1
+fi
 
 _send_qemu_cmd $h "nbd_server_start unix:$TEST_DIR/nbd" "(qemu)"
 _send_qemu_cmd $h "nbd_server_add -w drive0" "(qemu)"
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 4/7] iotests: extended timeout under Valgrind

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> As the iotests run longer under the Valgrind, the QEMU_COMM_TIMEOUT is
> to be increased in the test cases 028, 183 and 192 when running under
> the Valgrind.
> 
> Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>



Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v2 5/7] iotests: extend sleeping time under Valgrind

[Andrey Shinkevich]

To synchronize the time when QEMU is running longer under the Valgrind,
increase the sleeping time int the test 247.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/247 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247
index 546a794..1036a17 100755
--- a/tests/qemu-iotests/247
+++ b/tests/qemu-iotests/247
@@ -57,7 +57,11 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size
 {"execute":"block-commit",
  "arguments":{"device":"format-4", "top-node": "format-2", "base-node":"format-0", "job-id":"job0"}}
 EOF
-sleep 1
+if [ "${VALGRIND_QEMU}" == "y" ]; then
+    sleep 5
+else
+    sleep 1
+fi
 echo '{"execute":"quit"}'
 ) | $QEMU -qmp stdio -nographic -nodefaults \
     -blockdev file,node-name=file-0,filename=$TEST_IMG.0,auto-read-only=on \
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 5/7] iotests: extend sleeping time under Valgrind

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> To synchronize the time when QEMU is running longer under the Valgrind,
> increase the sleeping time int the test 247.

in the test

> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> ---
>   tests/qemu-iotests/247 | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/247 b/tests/qemu-iotests/247
> index 546a794..1036a17 100755
> --- a/tests/qemu-iotests/247
> +++ b/tests/qemu-iotests/247
> @@ -57,7 +57,11 @@ TEST_IMG="$TEST_IMG.4" _make_test_img $size
>   {"execute":"block-commit",
>    "arguments":{"device":"format-4", "top-node": "format-2", "base-node":"format-0", "job-id":"job0"}}
>   EOF
> -sleep 1
> +if [ "${VALGRIND_QEMU}" == "y" ]; then
> +    sleep 5
> +else
> +    sleep 1
> +fi
>   echo '{"execute":"quit"}'
>   ) | $QEMU -qmp stdio -nographic -nodefaults \
>       -blockdev file,node-name=file-0,filename=$TEST_IMG.0,auto-read-only=on \
> 


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v2 6/7] iotests: amend QEMU NBD process synchronization

[Andrey Shinkevich]

Processes are dying harder under the Valgring. It results in counting
the dying process as a newborn one. Make it sure that old NBD job get
finished before starting a new one.

Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/common.nbd | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
index 25fc9ff..e3dcc60 100644
--- a/tests/qemu-iotests/common.nbd
+++ b/tests/qemu-iotests/common.nbd
@@ -22,6 +22,7 @@
 nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
 nbd_tcp_addr="127.0.0.1"
 nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
+nbd_job_pid=""
 
 nbd_server_stop()
 {
@@ -33,6 +34,9 @@ nbd_server_stop()
             kill "$NBD_PID"
         fi
     fi
+    if [ -n "$nbd_job_pid" ] && kill -s 0 "$nbd_job_pid" 2>/dev/null; then
+        wait "$nbd_job_pid"
+    fi
     rm -f "$nbd_unix_socket"
 }
 
@@ -61,6 +65,7 @@ nbd_server_start_unix_socket()
 {
     nbd_server_stop
     $QEMU_NBD -v -t -k "$nbd_unix_socket" "$@" &
+    nbd_job_pid=$!
     nbd_server_wait_for_unix_socket $!
 }
 
@@ -105,5 +110,6 @@ nbd_server_start_tcp_socket()
 {
     nbd_server_stop
     $QEMU_NBD -v -t -b $nbd_tcp_addr -p $nbd_tcp_port "$@" &
+    nbd_job_pid=$!
     nbd_server_wait_for_tcp_socket $!
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 6/7] iotests: amend QEMU NBD process synchronization

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> Processes are dying harder under the Valgring. It results in counting
> the dying process as a newborn one. Make it sure that old NBD job get
> finished before starting a new one.
> 
> Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/common.nbd | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
> index 25fc9ff..e3dcc60 100644
> --- a/tests/qemu-iotests/common.nbd
> +++ b/tests/qemu-iotests/common.nbd
> @@ -22,6 +22,7 @@
>   nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
>   nbd_tcp_addr="127.0.0.1"
>   nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
> +nbd_job_pid=""
>   
>   nbd_server_stop()
>   {
> @@ -33,6 +34,9 @@ nbd_server_stop()
>               kill "$NBD_PID"
>           fi
>       fi

Honestly, I don't understand the problem from commit message, but anyway comment
should be added here, to mark that this is for valgrind... But you don't check for
VALGRIND enabled.. I it intentional?

> +    if [ -n "$nbd_job_pid" ] && kill -s 0 "$nbd_job_pid" 2>/dev/null; then
> +        wait "$nbd_job_pid"
> +    fi
>       rm -f "$nbd_unix_socket"
>   }
>   
> @@ -61,6 +65,7 @@ nbd_server_start_unix_socket()
>   {
>       nbd_server_stop
>       $QEMU_NBD -v -t -k "$nbd_unix_socket" "$@" &
> +    nbd_job_pid=$!
>       nbd_server_wait_for_unix_socket $!
>   }
>   
> @@ -105,5 +110,6 @@ nbd_server_start_tcp_socket()
>   {
>       nbd_server_stop
>       $QEMU_NBD -v -t -b $nbd_tcp_addr -p $nbd_tcp_port "$@" &
> +    nbd_job_pid=$!
>       nbd_server_wait_for_tcp_socket $!
>   }
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 6/7] iotests: amend QEMU NBD process synchronization

[Roman Kagan]

On Thu, Jun 13, 2019 at 12:59:53PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 11.06.2019 21:02, Andrey Shinkevich wrote:
> > Processes are dying harder under the Valgring. It results in counting
> > the dying process as a newborn one. Make it sure that old NBD job get
> > finished before starting a new one.
> > 
> > Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > ---
> >   tests/qemu-iotests/common.nbd | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
> > index 25fc9ff..e3dcc60 100644
> > --- a/tests/qemu-iotests/common.nbd
> > +++ b/tests/qemu-iotests/common.nbd
> > @@ -22,6 +22,7 @@
> >   nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
> >   nbd_tcp_addr="127.0.0.1"
> >   nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
> > +nbd_job_pid=""
> >   
> >   nbd_server_stop()
> >   {
> > @@ -33,6 +34,9 @@ nbd_server_stop()
> >               kill "$NBD_PID"
> >           fi
> >       fi
> 
> Honestly, I don't understand the problem from commit message, but anyway comment
> should be added here, to mark that this is for valgrind... But you don't check for
> VALGRIND enabled.. I it intentional?

It is.  The problem this patch fixes exists regardless of valgrind.
valgrind just makes it easier to notice.  See my reply to the patch
itself.

Roman.

> 
> > +    if [ -n "$nbd_job_pid" ] && kill -s 0 "$nbd_job_pid" 2>/dev/null; then
> > +        wait "$nbd_job_pid"
> > +    fi
> >       rm -f "$nbd_unix_socket"
> >   }
> >   
> > @@ -61,6 +65,7 @@ nbd_server_start_unix_socket()
> >   {
> >       nbd_server_stop
> >       $QEMU_NBD -v -t -k "$nbd_unix_socket" "$@" &
> > +    nbd_job_pid=$!
> >       nbd_server_wait_for_unix_socket $!
> >   }
> >   
> > @@ -105,5 +110,6 @@ nbd_server_start_tcp_socket()
> >   {
> >       nbd_server_stop
> >       $QEMU_NBD -v -t -b $nbd_tcp_addr -p $nbd_tcp_port "$@" &
> > +    nbd_job_pid=$!
> >       nbd_server_wait_for_tcp_socket $!
> >   }
> > 
> 
> 
> -- 
> Best regards,
> Vladimir

Re: [Qemu-devel] [PATCH v2 6/7] iotests: amend QEMU NBD process synchronization

[Roman Kagan]

On Tue, Jun 11, 2019 at 09:02:10PM +0300, Andrey Shinkevich wrote:
> Processes are dying harder under the Valgring. It results in counting
> the dying process as a newborn one. Make it sure that old NBD job get
> finished before starting a new one.

I think this log message is confusing.

The problem this patch addresses is that nbd_server_stop only sends a
signal to the nbd process and immediately returns, without waiting for
it to actually terminate.  The next operation is often starting a new
instance of nbd; this races with the termination of the old one, and may
result in various failures (like nbd_server_start_* taking the
terminating nbd as the one just started, or the starting nbd
encountering a busy listening socket).

Without valgrind the race window is very small and the problem didn't
surface for long.  However, under valgrind process termination takes
much longer so the race bites every test run.

Since nbd is run in a background job of the test, record the nbd pid at
the daemon start in a shell variable and perform a wait for it when
terminating it.

Roman.

> Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>  tests/qemu-iotests/common.nbd | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/qemu-iotests/common.nbd b/tests/qemu-iotests/common.nbd
> index 25fc9ff..e3dcc60 100644
> --- a/tests/qemu-iotests/common.nbd
> +++ b/tests/qemu-iotests/common.nbd
> @@ -22,6 +22,7 @@
>  nbd_unix_socket="${TEST_DIR}/qemu-nbd.sock"
>  nbd_tcp_addr="127.0.0.1"
>  nbd_pid_file="${TEST_DIR}/qemu-nbd.pid"
> +nbd_job_pid=""
>  
>  nbd_server_stop()
>  {
> @@ -33,6 +34,9 @@ nbd_server_stop()
>              kill "$NBD_PID"
>          fi
>      fi
> +    if [ -n "$nbd_job_pid" ] && kill -s 0 "$nbd_job_pid" 2>/dev/null; then
> +        wait "$nbd_job_pid"
> +    fi
>      rm -f "$nbd_unix_socket"
>  }
>  
> @@ -61,6 +65,7 @@ nbd_server_start_unix_socket()
>  {
>      nbd_server_stop
>      $QEMU_NBD -v -t -k "$nbd_unix_socket" "$@" &
> +    nbd_job_pid=$!
>      nbd_server_wait_for_unix_socket $!
>  }
>  
> @@ -105,5 +110,6 @@ nbd_server_start_tcp_socket()
>  {
>      nbd_server_stop
>      $QEMU_NBD -v -t -b $nbd_tcp_addr -p $nbd_tcp_port "$@" &
> +    nbd_job_pid=$!
>      nbd_server_wait_for_tcp_socket $!
>  }
> -- 
> 1.8.3.1
> 

[Qemu-devel] [PATCH v2 7/7] iotests: new file to suppress Valgrind errors

[Andrey Shinkevich]

The Valgrind tool reports about an uninitialised memory usage when the
initialization is actually not needed. For example, the buffer 'buf'
instantiated on a stack of the function guess_disk_lchs().
Let's use the Valgrind technology to suppress the unwanted reports by
adding the Valgrind specific format file valgrind.supp to the QEMU
project. The file content is extendable for future needs.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/common.rc     |  5 ++++-
 tests/qemu-iotests/valgrind.supp | 31 +++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 tests/qemu-iotests/valgrind.supp

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 3caaca4..9b74890 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -17,6 +17,8 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 
+readonly VALGRIND_SUPPRESS_ERRORS=./valgrind.supp
+
 SED=
 for sed in sed gsed; do
     ($sed --version | grep 'GNU sed') > /dev/null 2>&1
@@ -65,7 +67,8 @@ _qemu_proc_wrapper()
     local VALGRIND_LOGFILE="$1"
     shift
     if [ "${VALGRIND_QEMU}" == "y" ]; then
-        exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$@"
+        exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 \
+            --suppressions="${VALGRIND_SUPPRESS_ERRORS}" "$@"
     else
         exec "$@"
     fi
diff --git a/tests/qemu-iotests/valgrind.supp b/tests/qemu-iotests/valgrind.supp
new file mode 100644
index 0000000..78215b6
--- /dev/null
+++ b/tests/qemu-iotests/valgrind.supp
@@ -0,0 +1,31 @@
+#
+# Valgrind errors suppression file for QEMU iotests
+#
+# Copyright (c) 2019 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+{
+   hw/block/hd-geometry.c
+   Memcheck:Cond
+   fun:guess_disk_lchs
+   fun:hd_geometry_guess
+   fun:blkconf_geometry
+   ...
+   fun:device_set_realized
+   fun:property_set_bool
+   fun:object_property_set
+   fun:object_property_set_qobject
+   fun:object_property_set_bool
+}
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 7/7] iotests: new file to suppress Valgrind errors

[Vladimir Sementsov-Ogievskiy]

11.06.2019 21:02, Andrey Shinkevich wrote:
> The Valgrind tool reports about an uninitialised memory usage when the
> initialization is actually not needed. For example, the buffer 'buf'
> instantiated on a stack of the function guess_disk_lchs().

for convinience, you may add: "of the function guess_disk_lchs(), which
is then actually initialized by blk_pread_unthrottled()"

> Let's use the Valgrind technology to suppress the unwanted reports by
> adding the Valgrind specific format file valgrind.supp to the QEMU
> project. The file content is extendable for future needs.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/common.rc     |  5 ++++-
>   tests/qemu-iotests/valgrind.supp | 31 +++++++++++++++++++++++++++++++
>   2 files changed, 35 insertions(+), 1 deletion(-)
>   create mode 100644 tests/qemu-iotests/valgrind.supp
> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 3caaca4..9b74890 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -17,6 +17,8 @@
>   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   #
>   
> +readonly VALGRIND_SUPPRESS_ERRORS=./valgrind.supp

Why readonly?

I think it should be defined near and in same manner as VALGRIND_LOGFILE,
with use of TEST_DIR

> +
>   SED=
>   for sed in sed gsed; do
>       ($sed --version | grep 'GNU sed') > /dev/null 2>&1
> @@ -65,7 +67,8 @@ _qemu_proc_wrapper()
>       local VALGRIND_LOGFILE="$1"
>       shift
>       if [ "${VALGRIND_QEMU}" == "y" ]; then
> -        exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$@"
> +        exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 \
> +            --suppressions="${VALGRIND_SUPPRESS_ERRORS}" "$@"
>       else
>           exec "$@"
>       fi
> diff --git a/tests/qemu-iotests/valgrind.supp b/tests/qemu-iotests/valgrind.supp
> new file mode 100644
> index 0000000..78215b6
> --- /dev/null
> +++ b/tests/qemu-iotests/valgrind.supp
> @@ -0,0 +1,31 @@
> +#
> +# Valgrind errors suppression file for QEMU iotests
> +#
> +# Copyright (c) 2019 Virtuozzo International GmbH
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +{
> +   hw/block/hd-geometry.c
> +   Memcheck:Cond
> +   fun:guess_disk_lchs
> +   fun:hd_geometry_guess
> +   fun:blkconf_geometry
> +   ...
> +   fun:device_set_realized
> +   fun:property_set_bool
> +   fun:object_property_set
> +   fun:object_property_set_qobject
> +   fun:object_property_set_bool
> +}
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 7/7] iotests: new file to suppress Valgrind errors

[Andrey Shinkevich]

On 13/06/2019 13:06, Vladimir Sementsov-Ogievskiy wrote:
> 11.06.2019 21:02, Andrey Shinkevich wrote:
>> The Valgrind tool reports about an uninitialised memory usage when the
>> initialization is actually not needed. For example, the buffer 'buf'
>> instantiated on a stack of the function guess_disk_lchs().
> 
> for convinience, you may add: "of the function guess_disk_lchs(), which
> is then actually initialized by blk_pread_unthrottled()"
> 
>> Let's use the Valgrind technology to suppress the unwanted reports by
>> adding the Valgrind specific format file valgrind.supp to the QEMU
>> project. The file content is extendable for future needs.
>>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>    tests/qemu-iotests/common.rc     |  5 ++++-
>>    tests/qemu-iotests/valgrind.supp | 31 +++++++++++++++++++++++++++++++
>>    2 files changed, 35 insertions(+), 1 deletion(-)
>>    create mode 100644 tests/qemu-iotests/valgrind.supp
>>
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 3caaca4..9b74890 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -17,6 +17,8 @@
>>    # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>    #
>>    
>> +readonly VALGRIND_SUPPRESS_ERRORS=./valgrind.supp
> 
> Why readonly?
> 
> I think it should be defined near and in same manner as VALGRIND_LOGFILE,
> with use of TEST_DIR
> 

The new file 'valgrind.supp' is intended to be a part of the QEMU 
project. So, it will be located in the test directory tests/qemu-iotests.
The variable TEST_DIR may change the working directory. In that case, 
moving the project file will be a hassle.

Andrey

>> +
>>    SED=
>>    for sed in sed gsed; do
>>        ($sed --version | grep 'GNU sed') > /dev/null 2>&1
>> @@ -65,7 +67,8 @@ _qemu_proc_wrapper()
>>        local VALGRIND_LOGFILE="$1"
>>        shift
>>        if [ "${VALGRIND_QEMU}" == "y" ]; then
>> -        exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$@"
>> +        exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 \
>> +            --suppressions="${VALGRIND_SUPPRESS_ERRORS}" "$@"
>>        else
>>            exec "$@"
>>        fi
>> diff --git a/tests/qemu-iotests/valgrind.supp b/tests/qemu-iotests/valgrind.supp
>> new file mode 100644
>> index 0000000..78215b6
>> --- /dev/null
>> +++ b/tests/qemu-iotests/valgrind.supp
>> @@ -0,0 +1,31 @@
>> +#
>> +# Valgrind errors suppression file for QEMU iotests
>> +#
>> +# Copyright (c) 2019 Virtuozzo International GmbH
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +{
>> +   hw/block/hd-geometry.c
>> +   Memcheck:Cond
>> +   fun:guess_disk_lchs
>> +   fun:hd_geometry_guess
>> +   fun:blkconf_geometry
>> +   ...
>> +   fun:device_set_realized
>> +   fun:property_set_bool
>> +   fun:object_property_set
>> +   fun:object_property_set_qobject
>> +   fun:object_property_set_bool
>> +}
>>
> 
> 

-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH v2 7/7] iotests: new file to suppress Valgrind errors

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1536 bytes --]

On 6/24/19 11:55 AM, Andrey Shinkevich wrote:

>>> +++ b/tests/qemu-iotests/common.rc
>>> @@ -17,6 +17,8 @@
>>>    # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>    #
>>>    
>>> +readonly VALGRIND_SUPPRESS_ERRORS=./valgrind.supp
>>
>> Why readonly?
>>
>> I think it should be defined near and in same manner as VALGRIND_LOGFILE,
>> with use of TEST_DIR
>>
> 
> The new file 'valgrind.supp' is intended to be a part of the QEMU 
> project. So, it will be located in the test directory tests/qemu-iotests.
> The variable TEST_DIR may change the working directory. In that case, 
> moving the project file will be a hassle.

My personal thoughts are that no serious POSIX or bash shell script ever
uses readonly (it offers the illusion of security but cannot actually
back it up, and in reality ends up causing more bugs than it prevents
when your script breaks because you tried to modify a readonly
variable).  I've only ever dealt with one project that tried to use
readonly in earnest (the 'cygport' script for building Cygwin packages)
and it got in the way more than it saved me from bugs.

Declaring that VALGRIND_SUPPRESS_ERRORS is initialized hard-coded to ./
instead of relative to ${TEST_DIR} is orthogonal to whether you declare
that the variable VALGRIND_SUPPRESS_ERRORS can no longer be further
modified by the rest of the script.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v2 7/7] iotests: new file to suppress Valgrind errors

[Andrey Shinkevich]

On 24/06/2019 20:09, Eric Blake wrote:
> On 6/24/19 11:55 AM, Andrey Shinkevich wrote:
> 
>>>> +++ b/tests/qemu-iotests/common.rc
>>>> @@ -17,6 +17,8 @@
>>>>     # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>>     #
>>>>     
>>>> +readonly VALGRIND_SUPPRESS_ERRORS=./valgrind.supp
>>>
>>> Why readonly?
>>>
>>> I think it should be defined near and in same manner as VALGRIND_LOGFILE,
>>> with use of TEST_DIR
>>>
>>
>> The new file 'valgrind.supp' is intended to be a part of the QEMU
>> project. So, it will be located in the test directory tests/qemu-iotests.
>> The variable TEST_DIR may change the working directory. In that case,
>> moving the project file will be a hassle.
> 
> My personal thoughts are that no serious POSIX or bash shell script ever
> uses readonly (it offers the illusion of security but cannot actually
> back it up, and in reality ends up causing more bugs than it prevents
> when your script breaks because you tried to modify a readonly
> variable).  I've only ever dealt with one project that tried to use
> readonly in earnest (the 'cygport' script for building Cygwin packages)
> and it got in the way more than it saved me from bugs.
> 
> Declaring that VALGRIND_SUPPRESS_ERRORS is initialized hard-coded to ./
> instead of relative to ${TEST_DIR} is orthogonal to whether you declare
> that the variable VALGRIND_SUPPRESS_ERRORS can no longer be further
> modified by the rest of the script.
> 

Thank you Eric. I am flexible on that subject. If the path is relative 
to ${TEST_DIR}, should the file valgrind.supp be copied from ./ to the 
${TEST_DIR} by the script itself?

Andrey
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH v2 7/7] iotests: new file to suppress Valgrind errors

[Kevin Wolf]

Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben:
> The Valgrind tool reports about an uninitialised memory usage when the
> initialization is actually not needed. For example, the buffer 'buf'
> instantiated on a stack of the function guess_disk_lchs().

I would be careful with calling initialisation "not needed". It means
that the test case may not behave entirely determinstic because the
uninitialised memory can vary between runs.

In this specific case, I assume that guess_disk_lchs() is called for a
null block node, for which .bdrv_co_preadv by default returns without
actually writing to the buffer. Instead of ignoring the valgrind error,
we could instead pass read-zeroes=on to the null block driver to make
the test deterministic.

(Unfortunately, while adding the read-zeroes option, we didn't add it to
the QAPI schema, so it's not available yet in -blockdev. I'm going to
send a fix for that, but most of the problematic test cases probably
don't even use -blockdev.)

Kevin

Re: [Qemu-devel] [PATCH v2 7/7] iotests: new file to suppress Valgrind errors

[Andrey Shinkevich]

On 17/06/2019 14:45, Kevin Wolf wrote:
> Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben:
>> The Valgrind tool reports about an uninitialised memory usage when the
>> initialization is actually not needed. For example, the buffer 'buf'
>> instantiated on a stack of the function guess_disk_lchs().
> 
> I would be careful with calling initialisation "not needed". It means
> that the test case may not behave entirely determinstic because the
> uninitialised memory can vary between runs.\

I am going to amend the comment.

Andrey

> 
> In this specific case, I assume that guess_disk_lchs() is called for a
> null block node, for which .bdrv_co_preadv by default returns without
> actually writing to the buffer. Instead of ignoring the valgrind error,
> we could instead pass read-zeroes=on to the null block driver to make
> the test deterministic.

The buffer that the Valgrind complains of is initialized by the 
following function call blk_pread_unthrottled() that reads the first 
BDRV_SECTOR_SIZE bytes form a disk "to guess the disk logical geometry". 
The Valgrind does not recognize that way of initialization. I believe we 
do not need to zero the buffer instantiated on the stack just to make 
the Valgrind silent there.

Andrey

> 
> (Unfortunately, while adding the read-zeroes option, we didn't add it to
> the QAPI schema, so it's not available yet in -blockdev. I'm going to
> send a fix for that, but most of the problematic test cases probably
> don't even use -blockdev.)
> 
> Kevin
> 

-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH v2 7/7] iotests: new file to suppress Valgrind errors

[Kevin Wolf]

Am 24.06.2019 um 18:55 hat Andrey Shinkevich geschrieben:
> 
> 
> On 17/06/2019 14:45, Kevin Wolf wrote:
> > Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben:
> >> The Valgrind tool reports about an uninitialised memory usage when the
> >> initialization is actually not needed. For example, the buffer 'buf'
> >> instantiated on a stack of the function guess_disk_lchs().
> > 
> > I would be careful with calling initialisation "not needed". It means
> > that the test case may not behave entirely determinstic because the
> > uninitialised memory can vary between runs.\
> 
> I am going to amend the comment.
> 
> Andrey
> 
> > 
> > In this specific case, I assume that guess_disk_lchs() is called for a
> > null block node, for which .bdrv_co_preadv by default returns without
> > actually writing to the buffer. Instead of ignoring the valgrind error,
> > we could instead pass read-zeroes=on to the null block driver to make
> > the test deterministic.
> 
> The buffer that the Valgrind complains of is initialized by the 
> following function call blk_pread_unthrottled() that reads the first 
> BDRV_SECTOR_SIZE bytes form a disk "to guess the disk logical geometry". 
> The Valgrind does not recognize that way of initialization. I believe we 
> do not need to zero the buffer instantiated on the stack just to make 
> the Valgrind silent there.

My point is that blk_pread_unthrottled() with null-co/null-aio leaves
the buffer untouched if read-zeroes=off (which is the default). So yes,
valgrind is right, this memory is still uninitialised after
blk_pread_unthrottled().

Kevin