[Qemu-devel] [PATCH v5 0/6] Allow Valgrind checking all QEMU processes

[Qemu-devel] [PATCH v5 0/6] 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.

v5:
  01: The patch "block/nbd: NBDReply is used being uninitialized" was detached
      and taken into account in the patch "nbd: Initialize reply on failure"
      by Eric Blake.

v4:
  01: The patch "iotests: Set read-zeroes on in null block driver for Valgrind"
      was extended with new cases and issued as a separate series.
  02: The new patch "block/nbd: NBDReply is used being uninitialized" was
      added to resolve the failure of the iotest 083 run under Valgrind.

v3:
  01: The new function _casenotrun() was added to the common.rc bash
      script to notify the user of test cases dropped for some reason.
      Suggested by Kevin.
      Particularly, the notification about the nonexistent TMPDIR in
      the test 051 was added (noticed by Vladimir).
  02: The timeout in some test cases was extended for Valgrind because
      it differs when running on the ramdisk.
  03: Due to the common.nbd script has been changed with the commit
      b28f582c, the patch "iotests: amend QEMU NBD process synchronization"
      is actual no more. Note that QEMU_NBD is launched in the bash nested
      shell in the _qemu_nbd_wrapper() as it was before in common.rc.
  04: The patch "iotests: new file to suppress Valgrind errors" was dropped
      due to my superficial understanding of the work of the function
      blk_pread_unthrottled(). Special thanks to Kevin who shed the light
      on the null block driver involved. Now, the parameter 'read-zeroes=on'
      is passed to the null block driver to initialize the buffer in the
      function guess_disk_lchs() that the Valgrind was complaining to.

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.
  Discussed in the email thread with the message ID:
  <1560276131-683243-1-git-send-email-andrey.shinkevich@virtuozzo.com>

Andrey Shinkevich (6):
  iotests: allow Valgrind checking all QEMU processes
  iotests: exclude killed processes from running under  Valgrind
  iotests: Add casenotrun report to bash tests
  iotests: Valgrind fails with nonexistent directory
  iotests: extended timeout under Valgrind
  iotests: extend sleeping time under Valgrind

 tests/qemu-iotests/028       |  6 +++-
 tests/qemu-iotests/039       |  5 +++
 tests/qemu-iotests/039.out   | 30 +++--------------
 tests/qemu-iotests/051       |  4 +++
 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.rc | 76 +++++++++++++++++++++++++++++++++-----------
 12 files changed, 101 insertions(+), 62 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v5 1/6] 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
When QEMU-IO process is being killed, the shell report refers to the
text of the command in _qemu_io_wrapper(), which was modified with this
patch. So, the benchmark output for the tests 039, 061 and 137 is to be
changed also.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/039.out   | 30 ++++---------------
 tests/qemu-iotests/061.out   | 12 ++------
 tests/qemu-iotests/137.out   |  6 +---
 tests/qemu-iotests/common.rc | 69 ++++++++++++++++++++++++++++++++------------
 4 files changed, 59 insertions(+), 58 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 5502c3d..6e461a1 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,36 +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()
 {
-    "$QEMU_NBD_PROG" --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" \
-                     $QEMU_NBD_OPTIONS "$@"
+    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
+    (
+        _qemu_proc_wrapper "${VALGRIND_LOGFILE}" "$QEMU_NBD_PROG" \
+            --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" $QEMU_NBD_OPTIONS "$@"
+    )
+    RETVAL=$?
+    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
+    return $RETVAL
 }
 
 _qemu_vxhs_wrapper()
 {
+    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
     (
         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

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

[Andrey Shinkevich]

 The Valgrind tool fails to manage its termination when QEMU raises the
 signal SIGKILL in the multi-threaded process. The bug has been
 reported to the Valgrind maintainers and was registered as Bug 409141.
 Let's exclude such test cases from running under the Valgrind until
 new release of it because checking for the memory issues is covered by
 other test cases.

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

[Qemu-devel] [PATCH v5 3/6] iotests: Add casenotrun report to bash tests

[Andrey Shinkevich]

The new function _casenotrun() is to be invoked if a test case cannot
be run for some reason. The user will be notified by a message passed
to the function.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/common.rc | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 6e461a1..1089050 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -428,6 +428,13 @@ _notrun()
     exit
 }
 
+# bail out, setting up .casenotrun file
+#
+_casenotrun()
+{
+    echo "    [case not run] $*" >>"$OUTPUT_DIR/$seq.casenotrun"
+}
+
 # just plain bail out
 #
 _fail()
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 4/6] iotests: Valgrind fails 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 and notify the user of it.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/051 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index ce942a5..f8141ca 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -377,6 +377,10 @@ 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
+if [ "${VALGRIND_QEMU}" == "y" ]; then
+    _casenotrun "Valgrind needs a valid TMPDIR for itself"
+fi
+VALGRIND_QEMU="" \
 TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on
 
 # Using snapshot=on together with read-only=on
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 5/6] 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@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..71301ec 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=7
+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..04fb344 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=4
+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..0344322 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=7
+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

[Qemu-devel] [PATCH v5 6/6] iotests: extend sleeping time under Valgrind

[Andrey Shinkevich]

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

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..c853b73 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 10
+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 v5 0/6] Allow Valgrind checking all QEMU processes

[Andrey Shinkevich]

PINGING...

On 19/07/2019 19:30, Andrey Shinkevich wrote:
> 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.
> 
> v5:
>    01: The patch "block/nbd: NBDReply is used being uninitialized" was detached
>        and taken into account in the patch "nbd: Initialize reply on failure"
>        by Eric Blake.
> 
> v4:
>    01: The patch "iotests: Set read-zeroes on in null block driver for Valgrind"
>        was extended with new cases and issued as a separate series.
>    02: The new patch "block/nbd: NBDReply is used being uninitialized" was
>        added to resolve the failure of the iotest 083 run under Valgrind.
> 
> v3:
>    01: The new function _casenotrun() was added to the common.rc bash
>        script to notify the user of test cases dropped for some reason.
>        Suggested by Kevin.
>        Particularly, the notification about the nonexistent TMPDIR in
>        the test 051 was added (noticed by Vladimir).
>    02: The timeout in some test cases was extended for Valgrind because
>        it differs when running on the ramdisk.
>    03: Due to the common.nbd script has been changed with the commit
>        b28f582c, the patch "iotests: amend QEMU NBD process synchronization"
>        is actual no more. Note that QEMU_NBD is launched in the bash nested
>        shell in the _qemu_nbd_wrapper() as it was before in common.rc.
>    04: The patch "iotests: new file to suppress Valgrind errors" was dropped
>        due to my superficial understanding of the work of the function
>        blk_pread_unthrottled(). Special thanks to Kevin who shed the light
>        on the null block driver involved. Now, the parameter 'read-zeroes=on'
>        is passed to the null block driver to initialize the buffer in the
>        function guess_disk_lchs() that the Valgrind was complaining to.
> 
> 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.
>    Discussed in the email thread with the message ID:
>    <1560276131-683243-1-git-send-email-andrey.shinkevich@virtuozzo.com>
> 
> Andrey Shinkevich (6):
>    iotests: allow Valgrind checking all QEMU processes
>    iotests: exclude killed processes from running under  Valgrind
>    iotests: Add casenotrun report to bash tests
>    iotests: Valgrind fails with nonexistent directory
>    iotests: extended timeout under Valgrind
>    iotests: extend sleeping time under Valgrind
> 
>   tests/qemu-iotests/028       |  6 +++-
>   tests/qemu-iotests/039       |  5 +++
>   tests/qemu-iotests/039.out   | 30 +++--------------
>   tests/qemu-iotests/051       |  4 +++
>   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.rc | 76 +++++++++++++++++++++++++++++++++-----------
>   12 files changed, 101 insertions(+), 62 deletions(-)
> 

-- 
With the best regards,
Andrey Shinkevich

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

[John Snow]

On 7/19/19 12:30 PM, 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
> When QEMU-IO process is being killed, the shell report refers to the
> text of the command in _qemu_io_wrapper(), which was modified with this
> patch. So, the benchmark output for the tests 039, 061 and 137 is to be
> changed also.
> 

Oh, weird. "VALGRIND_QEMU=y" actually has just meant ... valgrind
qemu-io. OK.

> 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 | 69 ++++++++++++++++++++++++++++++++------------
>  4 files changed, 59 insertions(+), 58 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 5502c3d..6e461a1 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
> +}
> +

Why do we need a second wrapper? I get nervous with each new wrapper we
make, because I feel like it has unintended consequences with pipe
handling and so on in the test dispatcher scripts.

> +_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 "$@"

Can we not inline that logic here? especially because:

>      )
> +    RETVAL=$?
> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL

We're already making valgrind calls here anyway.

> +    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,36 +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()
>  {
> -    "$QEMU_NBD_PROG" --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" \
> -                     $QEMU_NBD_OPTIONS "$@"
> +    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
> +    (
> +        _qemu_proc_wrapper "${VALGRIND_LOGFILE}" "$QEMU_NBD_PROG" \
> +            --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" $QEMU_NBD_OPTIONS "$@"
> +    )
> +    RETVAL=$?
> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
> +    return $RETVAL
>  }
>  
>  _qemu_vxhs_wrapper()
>  {
> +    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
>      (
>          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
> 

Only other thought: at the moment, valgrind turns on valgrind for
qemu-io; this wraps many more tools and potentially slows down iotests a
lot. If there are problems, we might want easy access to turn /some/ but
not all of these options off again.


do we want to be able to specify which subprocesses we use valgrind on,
Perhaps with environment variables for fine-tuning?

QEMU_VALGRIND_QEMU_IO = "off"
QEMU_VALGRIND_QEMU = "off"
QEMU_VALGRIND_NBD = "off"

(Only an idle question, not necessary for this series IMO.)

Re: [Qemu-devel] [Qemu-block] [PATCH v5 2/6] iotests: exclude killed processes from running under Valgrind

[John Snow]

On 7/19/19 12:30 PM, Andrey Shinkevich wrote:
>  The Valgrind tool fails to manage its termination when QEMU raises the
>  signal SIGKILL in the multi-threaded process. The bug has been
>  reported to the Valgrind maintainers and was registered as Bug 409141.
>  Let's exclude such test cases from running under the Valgrind until
>  new release of it because checking for the memory issues is covered by
>  other test cases.
> 

Link to the bug in the commit message, please:
https://bugs.kde.org/show_bug.cgi?id=409141

...Hey! It looks like they may have fixed it. However, we don't know if
the user has a properly fixed version of valgrind or not.

How long do we keep these workarounds in-tree?

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

This seems like the sort of thing that's going to get forgotten about
quickly. It's not clear (if you're reading the test source) why we're
setting VALGRIND_QEMU="" before these invocations.

Is it possible to create some kind of _NO_VALGRIND() shim that has some
comment in it, like:

# Valgrind bug #409141 https://bugs.kde.org/show_bug.cgi?id=409141
# Until valgrind 3.16+ is ubiquitous, we must work around a hang in
# valgrind when issuing sigkill. Disable valgrind for this invocation.

VALGRIND_QEMU="" exec "$@"

(something like that.)

This way:

(1) The workaround is being clearly demonstrated as a bit of a hack
(2) The shim explains what the hack is for
(3) When we decide we no longer need the hack, we can pull it out of a
central location and easily grep to find callers of the shim.


> +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" \
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v5 3/6] iotests: Add casenotrun report to bash tests

[John Snow]

On 7/19/19 12:30 PM, Andrey Shinkevich wrote:
> The new function _casenotrun() is to be invoked if a test case cannot
> be run for some reason. The user will be notified by a message passed
> to the function.
> 

Oh, I assume this is a sub-test granularity; if we need to skip
individual items.

I'm good with this, but we should CC Cleber Rosa, who has struggled
against this in the past, too.

> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>  tests/qemu-iotests/common.rc | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 6e461a1..1089050 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -428,6 +428,13 @@ _notrun()
>      exit
>  }
>  
> +# bail out, setting up .casenotrun file
> +#
> +_casenotrun()
> +{
> +    echo "    [case not run] $*" >>"$OUTPUT_DIR/$seq.casenotrun"
> +}
> +
>  # just plain bail out
>  #
>  _fail()
> 

seems fine to me otherwise.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v5 4/6] iotests: Valgrind fails with nonexistent directory

[John Snow]

On 7/19/19 12:30 PM, 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 and notify the user of it.
> 
> Suggested-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>  tests/qemu-iotests/051 | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
> index ce942a5..f8141ca 100755
> --- a/tests/qemu-iotests/051
> +++ b/tests/qemu-iotests/051
> @@ -377,6 +377,10 @@ 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
> +if [ "${VALGRIND_QEMU}" == "y" ]; then
> +    _casenotrun "Valgrind needs a valid TMPDIR for itself"
> +fi
> +VALGRIND_QEMU="" \
>  TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on
>  
>  # Using snapshot=on together with read-only=on
> 

The only other way around this would be a complicated mechanism to set
the TMPDIR for valgrind's sub-processes only, with e.g.

valgrind ... env TMPDIR=/nonexistent qemu ...

... It's probably not worth trying to concoct such a thing; but I
suppose it is possible. You'd have to set up a generic layer for setting
environment variables, then in the qemu shim, you could either set them
directly (non-valgrind invocation) or set them as part of the valgrind
command-line.

Or you could just take my R-B:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v5 5/6] iotests: extended timeout under Valgrind

[John Snow]

On 7/19/19 12:30 PM, 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>
> ---
>  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..71301ec 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=7
> +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..04fb344 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=4
> +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..0344322 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=7
> +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)"
> 

I guess we're adding some more magic numbers to join the magic numbers
we already have.

Ah, well, perfection is a good way to make sure nothing good ever happens:

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v5 6/6] iotests: extend sleeping time under Valgrind

[John Snow]

On 7/19/19 12:30 PM, Andrey Shinkevich wrote:
> To synchronize the time when QEMU is running longer under the Valgrind,
> increase the sleeping time in the test 247.
> 
> 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..c853b73 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 10
> +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 \
> 

This makes me nervous, though. Won't this race terribly? (Wait, why
doesn't it race already?)

Re: [Qemu-devel] [PATCH v5 0/6] Allow Valgrind checking all QEMU processes

[Cleber Rosa]

On Fri, Jul 19, 2019 at 07:30:10PM +0300, Andrey Shinkevich wrote:
> 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.
>

FIY, this looks very similar (in purpose) to:

   https://avocado-framework.readthedocs.io/en/71.0/WrapProcess.html

And in fact Valgrind was one of the original motivations:

   https://github.com/avocado-framework/avocado/blob/master/examples/wrappers/valgrind.sh

Maybe this can be helpful for the Python based iotests.

- Cleber.

> v5:
>   01: The patch "block/nbd: NBDReply is used being uninitialized" was detached
>       and taken into account in the patch "nbd: Initialize reply on failure"
>       by Eric Blake.
> 
> v4:
>   01: The patch "iotests: Set read-zeroes on in null block driver for Valgrind"
>       was extended with new cases and issued as a separate series.
>   02: The new patch "block/nbd: NBDReply is used being uninitialized" was
>       added to resolve the failure of the iotest 083 run under Valgrind.
> 
> v3:
>   01: The new function _casenotrun() was added to the common.rc bash
>       script to notify the user of test cases dropped for some reason.
>       Suggested by Kevin.
>       Particularly, the notification about the nonexistent TMPDIR in
>       the test 051 was added (noticed by Vladimir).
>   02: The timeout in some test cases was extended for Valgrind because
>       it differs when running on the ramdisk.
>   03: Due to the common.nbd script has been changed with the commit
>       b28f582c, the patch "iotests: amend QEMU NBD process synchronization"
>       is actual no more. Note that QEMU_NBD is launched in the bash nested
>       shell in the _qemu_nbd_wrapper() as it was before in common.rc.
>   04: The patch "iotests: new file to suppress Valgrind errors" was dropped
>       due to my superficial understanding of the work of the function
>       blk_pread_unthrottled(). Special thanks to Kevin who shed the light
>       on the null block driver involved. Now, the parameter 'read-zeroes=on'
>       is passed to the null block driver to initialize the buffer in the
>       function guess_disk_lchs() that the Valgrind was complaining to.
> 
> 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.
>   Discussed in the email thread with the message ID:
>   <1560276131-683243-1-git-send-email-andrey.shinkevich@virtuozzo.com>
> 
> Andrey Shinkevich (6):
>   iotests: allow Valgrind checking all QEMU processes
>   iotests: exclude killed processes from running under  Valgrind
>   iotests: Add casenotrun report to bash tests
>   iotests: Valgrind fails with nonexistent directory
>   iotests: extended timeout under Valgrind
>   iotests: extend sleeping time under Valgrind
> 
>  tests/qemu-iotests/028       |  6 +++-
>  tests/qemu-iotests/039       |  5 +++
>  tests/qemu-iotests/039.out   | 30 +++--------------
>  tests/qemu-iotests/051       |  4 +++
>  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.rc | 76 +++++++++++++++++++++++++++++++++-----------
>  12 files changed, 101 insertions(+), 62 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH v5 3/6] iotests: Add casenotrun report to bash tests

[Cleber Rosa]

On Thu, Aug 15, 2019 at 08:44:11PM -0400, John Snow wrote:
> 
> 
> On 7/19/19 12:30 PM, Andrey Shinkevich wrote:
> > The new function _casenotrun() is to be invoked if a test case cannot
> > be run for some reason. The user will be notified by a message passed
> > to the function.
> > 
> 
> Oh, I assume this is a sub-test granularity; if we need to skip
> individual items.
> 
> I'm good with this, but we should CC Cleber Rosa, who has struggled
> against this in the past, too.
>

The discussion I was involved in was not that much about skipping
tests per se, but about how to determine if a test should be skipped
or not.  At that time, we proposed an integration with the build
system, but the downside (and the reason for not pushing it forward)
was the requirement to run the iotest outside of a build tree.

> > Suggested-by: Kevin Wolf <kwolf@redhat.com>
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > ---
> >  tests/qemu-iotests/common.rc | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> > index 6e461a1..1089050 100644
> > --- a/tests/qemu-iotests/common.rc
> > +++ b/tests/qemu-iotests/common.rc
> > @@ -428,6 +428,13 @@ _notrun()
> >      exit
> >  }
> >  
> > +# bail out, setting up .casenotrun file
> > +#
> > +_casenotrun()
> > +{
> > +    echo "    [case not run] $*" >>"$OUTPUT_DIR/$seq.casenotrun"
> > +}
> > +
> >  # just plain bail out
> >  #
> >  _fail()
> > 
> 
> seems fine to me otherwise.
> 
> Reviewed-by: John Snow <jsnow@redhat.com>

Yeah, this also LGTM.

Reviewed-by: Cleber Rosa <crosa@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH v5 6/6] iotests: extend sleeping time under Valgrind

[Vladimir Sementsov-Ogievskiy]

16.08.2019 4:01, John Snow wrote:
> 
> 
> On 7/19/19 12:30 PM, Andrey Shinkevich wrote:
>> To synchronize the time when QEMU is running longer under the Valgrind,
>> increase the sleeping time in the test 247.
>>
>> 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..c853b73 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 10
>> +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 \
>>
> 
> This makes me nervous, though. Won't this race terribly? (Wait, why
> doesn't it race already?)
> 

Hmm, however it works somehow. I'm afraid that everything with "sleep" is definitely racy..
Or what do you mean?

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [Qemu-block] [PATCH v5 6/6] iotests: extend sleeping time under Valgrind

[Andrey Shinkevich]

On 16/08/2019 04:01, John Snow wrote:
> 
> 
> On 7/19/19 12:30 PM, Andrey Shinkevich wrote:
>> To synchronize the time when QEMU is running longer under the Valgrind,
>> increase the sleeping time in the test 247.
>>
>> 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..c853b73 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 10
>> +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 \
>>
> 
> This makes me nervous, though. Won't this race terribly? (Wait, why
> doesn't it race already?)
> 
It maybe better to rewrite this test in Python.
To let it work under Valgrind in the current implementation, I reserved 
more seconds. We could have this tolerance just for the test.

Andrey
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [PATCH v5 0/6] Allow Valgrind checking all QEMU processes

[Andrey Shinkevich]

On 16/08/2019 23:05, Cleber Rosa wrote:
> On Fri, Jul 19, 2019 at 07:30:10PM +0300, Andrey Shinkevich wrote:
>> 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.
>>
> 
> FIY, this looks very similar (in purpose) to:
> 
>     https://avocado-framework.readthedocs.io/en/71.0/WrapProcess.html
> 
> And in fact Valgrind was one of the original motivations:
> 
>     https://github.com/avocado-framework/avocado/blob/master/examples/wrappers/valgrind.sh
> 
> Maybe this can be helpful for the Python based iotests.
> 
> - Cleber.
> 

Thank you Cleber for the advice. That is the way I actually ran Python 
iotests under Valgrind on my host and discovered some issues with them 
already.

Andrey

>> v5:
>>    01: The patch "block/nbd: NBDReply is used being uninitialized" was detached
>>        and taken into account in the patch "nbd: Initialize reply on failure"
>>        by Eric Blake.
>>
>> v4:
>>    01: The patch "iotests: Set read-zeroes on in null block driver for Valgrind"
>>        was extended with new cases and issued as a separate series.
>>    02: The new patch "block/nbd: NBDReply is used being uninitialized" was
>>        added to resolve the failure of the iotest 083 run under Valgrind.
>>
>> v3:
>>    01: The new function _casenotrun() was added to the common.rc bash
>>        script to notify the user of test cases dropped for some reason.
>>        Suggested by Kevin.
>>        Particularly, the notification about the nonexistent TMPDIR in
>>        the test 051 was added (noticed by Vladimir).
>>    02: The timeout in some test cases was extended for Valgrind because
>>        it differs when running on the ramdisk.
>>    03: Due to the common.nbd script has been changed with the commit
>>        b28f582c, the patch "iotests: amend QEMU NBD process synchronization"
>>        is actual no more. Note that QEMU_NBD is launched in the bash nested
>>        shell in the _qemu_nbd_wrapper() as it was before in common.rc.
>>    04: The patch "iotests: new file to suppress Valgrind errors" was dropped
>>        due to my superficial understanding of the work of the function
>>        blk_pread_unthrottled(). Special thanks to Kevin who shed the light
>>        on the null block driver involved. Now, the parameter 'read-zeroes=on'
>>        is passed to the null block driver to initialize the buffer in the
>>        function guess_disk_lchs() that the Valgrind was complaining to.
>>
>> 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.
>>    Discussed in the email thread with the message ID:
>>    <1560276131-683243-1-git-send-email-andrey.shinkevich@virtuozzo.com>
>>
>> Andrey Shinkevich (6):
>>    iotests: allow Valgrind checking all QEMU processes
>>    iotests: exclude killed processes from running under  Valgrind
>>    iotests: Add casenotrun report to bash tests
>>    iotests: Valgrind fails with nonexistent directory
>>    iotests: extended timeout under Valgrind
>>    iotests: extend sleeping time under Valgrind
>>
>>   tests/qemu-iotests/028       |  6 +++-
>>   tests/qemu-iotests/039       |  5 +++
>>   tests/qemu-iotests/039.out   | 30 +++--------------
>>   tests/qemu-iotests/051       |  4 +++
>>   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.rc | 76 +++++++++++++++++++++++++++++++++-----------
>>   12 files changed, 101 insertions(+), 62 deletions(-)
>>
>> -- 
>> 1.8.3.1
>>
>>

-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [Qemu-block] [PATCH v5 3/6] iotests: Add casenotrun report to bash tests

[Andrey Shinkevich]

On 16/08/2019 23:33, Cleber Rosa wrote:
> On Thu, Aug 15, 2019 at 08:44:11PM -0400, John Snow wrote:
>>
>>
>> On 7/19/19 12:30 PM, Andrey Shinkevich wrote:
>>> The new function _casenotrun() is to be invoked if a test case cannot
>>> be run for some reason. The user will be notified by a message passed
>>> to the function.
>>>
>>
>> Oh, I assume this is a sub-test granularity; if we need to skip
>> individual items.
>>
>> I'm good with this, but we should CC Cleber Rosa, who has struggled
>> against this in the past, too.
>>
> 
> The discussion I was involved in was not that much about skipping
> tests per se, but about how to determine if a test should be skipped
> or not.  At that time, we proposed an integration with the build
> system, but the downside (and the reason for not pushing it forward)
> was the requirement to run the iotest outside of a build tree.
> 
>>> Suggested-by: Kevin Wolf <kwolf@redhat.com>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> ---
>>>   tests/qemu-iotests/common.rc | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>>> index 6e461a1..1089050 100644
>>> --- a/tests/qemu-iotests/common.rc
>>> +++ b/tests/qemu-iotests/common.rc
>>> @@ -428,6 +428,13 @@ _notrun()
>>>       exit
>>>   }
>>>   
>>> +# bail out, setting up .casenotrun file
>>> +#
>>> +_casenotrun()
>>> +{
>>> +    echo "    [case not run] $*" >>"$OUTPUT_DIR/$seq.casenotrun"
>>> +}
>>> +
>>>   # just plain bail out
>>>   #
>>>   _fail()
>>>
>>
>> seems fine to me otherwise.
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
> 
> Yeah, this also LGTM.
> 
> Reviewed-by: Cleber Rosa <crosa@redhat.com>
> 

Thank you very much for your reviews. Please note that the function 
_casenotrun() works as a notifier only as it was done for the Python 
based iotests. It is a caller responsibility to skip running a 
particular test with all relevant logic. I will supply the comment in v6 
and will keep your 'Reviewed-by' if there are no objections ))

Andrey
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [Qemu-block] [PATCH v5 4/6] iotests: Valgrind fails with nonexistent directory

[Andrey Shinkevich]

On 16/08/2019 03:55, John Snow wrote:
> 
> 
> On 7/19/19 12:30 PM, 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 and notify the user of it.
>>
>> Suggested-by: Kevin Wolf <kwolf@redhat.com>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/051 | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
>> index ce942a5..f8141ca 100755
>> --- a/tests/qemu-iotests/051
>> +++ b/tests/qemu-iotests/051
>> @@ -377,6 +377,10 @@ 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
>> +if [ "${VALGRIND_QEMU}" == "y" ]; then
>> +    _casenotrun "Valgrind needs a valid TMPDIR for itself"
>> +fi
>> +VALGRIND_QEMU="" \
>>   TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on
>>   
>>   # Using snapshot=on together with read-only=on
>>
> 
> The only other way around this would be a complicated mechanism to set
> the TMPDIR for valgrind's sub-processes only, with e.g.
> 
> valgrind ... env TMPDIR=/nonexistent qemu ...
> 
> ... It's probably not worth trying to concoct such a thing; but I
> suppose it is possible. You'd have to set up a generic layer for setting
> environment variables, then in the qemu shim, you could either set them
> directly (non-valgrind invocation) or set them as part of the valgrind
> command-line.
> 
> Or you could just take my R-B:
> 
> Reviewed-by: John Snow <jsnow@redhat.com>
> 

Thanks again John for your review and the advice.
Probably, it doesn't worth efforts to manage that case because QEMU 
should fail anyway with the error message "Could not get temporary 
filename: No such file or directory". So, we would not benefit much from 
that run. We have other test cases that cover the main functionality. 
It's just to check the QEMU error path for possible memory issues. Shall we?

Andrey
-- 
With the best regards,
Andrey Shinkevich

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

[Andrey Shinkevich]

On 16/08/2019 01:49, John Snow wrote:
> 
> 
> On 7/19/19 12:30 PM, 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
>> When QEMU-IO process is being killed, the shell report refers to the
>> text of the command in _qemu_io_wrapper(), which was modified with this
>> patch. So, the benchmark output for the tests 039, 061 and 137 is to be
>> changed also.
>>
> 
> Oh, weird. "VALGRIND_QEMU=y" actually has just meant ... valgrind
> qemu-io. OK.
> 
>> 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 | 69 ++++++++++++++++++++++++++++++++------------
>>   4 files changed, 59 insertions(+), 58 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 5502c3d..6e461a1 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
>> +}
>> +
> 
> Why do we need a second wrapper? I get nervous with each new wrapper we
> make, because I feel like it has unintended consequences with pipe
> handling and so on in the test dispatcher scripts.
> 

The _qemu_proc_wrapper() is acctually the function rather than a new 
wrapper. The suffix _wrapper can be changed to mislead a reader not. The 
routine code originates from the _qemu_io_wrapper() to repeat the same 
functionality for the QEMU processes other than qemu-io and to keep the 
uniformity. It is also true for the function _qemu_proc_valgrind_log() 
that dumps Valgrind reports in case of errors. The behavior is the same 
to one in the _qemu_io_wrapper(). Actually, nothing new except that 
_qemu_nbd_wrapper() gets the subshell exec to keep the uniformity.

Andrey

>> +_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 "$@"
> 
> Can we not inline that logic here? especially because:
> 
>>       )
>> +    RETVAL=$?
>> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
> 
> We're already making valgrind calls here anyway.
> 

We can of cause, but we would repeat that for all other 
_qemu_*_wrapper() functions as well. The _qemu_proc_wrapper() runs a 
process under the Valgrind if VALGRIND_QEMU=y and the 
_qemu_proc_valgrind_log() dumps Valgrind error reports, if any and 
removes the Valgrind temporary log file. Again, the same pattern is used 
as it is in the current implementation of _qemu_io_wrapper(). I have 
just copied the functionality to all other QEMU wrapper functions.

Andrey

>> +    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,36 +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()
>>   {
>> -    "$QEMU_NBD_PROG" --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" \
>> -                     $QEMU_NBD_OPTIONS "$@"
>> +    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
>> +    (
>> +        _qemu_proc_wrapper "${VALGRIND_LOGFILE}" "$QEMU_NBD_PROG" \
>> +            --pid-file="${QEMU_TEST_DIR}/qemu-nbd.pid" $QEMU_NBD_OPTIONS "$@"
>> +    )
>> +    RETVAL=$?
>> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
>> +    return $RETVAL
>>   }
>>   
>>   _qemu_vxhs_wrapper()
>>   {
>> +    local VALGRIND_LOGFILE="${TEST_DIR}"/$$.valgrind
>>       (
>>           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
>>
> 
> Only other thought: at the moment, valgrind turns on valgrind for
> qemu-io; this wraps many more tools and potentially slows down iotests a
> lot. If there are problems, we might want easy access to turn /some/ but
> not all of these options off again.
> 
> 
> do we want to be able to specify which subprocesses we use valgrind on,
> Perhaps with environment variables for fine-tuning?
> 
> QEMU_VALGRIND_QEMU_IO = "off"
> QEMU_VALGRIND_QEMU = "off"
> QEMU_VALGRIND_NBD = "off"
> 
> (Only an idle question, not necessary for this series IMO.)
> 

-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [Qemu-block] [PATCH v5 6/6] iotests: extend sleeping time under Valgrind

[John Snow]

On 8/23/19 11:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> 16.08.2019 4:01, John Snow wrote:
>>
>>
>> On 7/19/19 12:30 PM, Andrey Shinkevich wrote:
>>> To synchronize the time when QEMU is running longer under the Valgrind,
>>> increase the sleeping time in the test 247.
>>>
>>> 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..c853b73 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 10
>>> +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 \
>>>
>>
>> This makes me nervous, though. Won't this race terribly? (Wait, why
>> doesn't it race already?)
>>
> 
> Hmm, however it works somehow. I'm afraid that everything with "sleep" is definitely racy..
> Or what do you mean?
> 

Right -- anything with a sleep is already at risk for racing.

What I am picking up on here is that with valgrind, there is an even
greater computational overhead that's much harder to predict, so I was
wondering how these values were determined.

(I wouldn't withhold an RB for that alone -- the sleeps are existing
problems.)

What I moved on to wondering in particular is why test 247 doesn't
already have race problems, because it looks quite fragile.

Neither of these are really Andrey's problems; I was just surprised
momentarily that I don't see 247 fail more often already, as-is.

--js

Re: [Qemu-devel] [Qemu-block] [PATCH v5 4/6] iotests: Valgrind fails with nonexistent directory

[John Snow]

On 8/25/19 11:24 AM, Andrey Shinkevich wrote:
> 
> 
> On 16/08/2019 03:55, John Snow wrote:
>>
>>
>> On 7/19/19 12:30 PM, 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 and notify the user of it.
>>>
>>> Suggested-by: Kevin Wolf <kwolf@redhat.com>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> ---
>>>   tests/qemu-iotests/051 | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
>>> index ce942a5..f8141ca 100755
>>> --- a/tests/qemu-iotests/051
>>> +++ b/tests/qemu-iotests/051
>>> @@ -377,6 +377,10 @@ 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
>>> +if [ "${VALGRIND_QEMU}" == "y" ]; then
>>> +    _casenotrun "Valgrind needs a valid TMPDIR for itself"
>>> +fi
>>> +VALGRIND_QEMU="" \
>>>   TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on
>>>   
>>>   # Using snapshot=on together with read-only=on
>>>
>>
>> The only other way around this would be a complicated mechanism to set
>> the TMPDIR for valgrind's sub-processes only, with e.g.
>>
>> valgrind ... env TMPDIR=/nonexistent qemu ...
>>
>> ... It's probably not worth trying to concoct such a thing; but I
>> suppose it is possible. You'd have to set up a generic layer for setting
>> environment variables, then in the qemu shim, you could either set them
>> directly (non-valgrind invocation) or set them as part of the valgrind
>> command-line.
>>
>> Or you could just take my R-B:
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>>
> 
> Thanks again John for your review and the advice.
> Probably, it doesn't worth efforts to manage that case because QEMU 
> should fail anyway with the error message "Could not get temporary 
> filename: No such file or directory". So, we would not benefit much from 
> that run. We have other test cases that cover the main functionality. 
> It's just to check the QEMU error path for possible memory issues. Shall we?
> 
> Andrey
> 

Yeah, don't bother with this for now. I just have a personal compulsion
to try to concretely measure how much work it would take to avoid a
"hack" and then make my decision.

You're free to just take the R-B :)

--js

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

[John Snow]

On 8/25/19 11:26 AM, Andrey Shinkevich wrote:
> 
> 
> On 16/08/2019 01:49, John Snow wrote:
>>
>>
>> On 7/19/19 12:30 PM, 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
>>> When QEMU-IO process is being killed, the shell report refers to the
>>> text of the command in _qemu_io_wrapper(), which was modified with this
>>> patch. So, the benchmark output for the tests 039, 061 and 137 is to be
>>> changed also.
>>>
>>
>> Oh, weird. "VALGRIND_QEMU=y" actually has just meant ... valgrind
>> qemu-io. OK.
>>
>>> 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 | 69 ++++++++++++++++++++++++++++++++------------
>>>   4 files changed, 59 insertions(+), 58 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 5502c3d..6e461a1 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
>>> +}
>>> +
>>
>> Why do we need a second wrapper? I get nervous with each new wrapper we
>> make, because I feel like it has unintended consequences with pipe
>> handling and so on in the test dispatcher scripts.
>>
> 
> The _qemu_proc_wrapper() is acctually the function rather than a new 
> wrapper. The suffix _wrapper can be changed to mislead a reader not. The 
> routine code originates from the _qemu_io_wrapper() to repeat the same 
> functionality for the QEMU processes other than qemu-io and to keep the 
> uniformity. It is also true for the function _qemu_proc_valgrind_log() 
> that dumps Valgrind reports in case of errors. The behavior is the same 
> to one in the _qemu_io_wrapper(). Actually, nothing new except that 
> _qemu_nbd_wrapper() gets the subshell exec to keep the uniformity.
> 
> Andrey
> 
>>> +_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 "$@"
>>
>> Can we not inline that logic here? especially because:
>>
>>>       )
>>> +    RETVAL=$?
>>> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
>>
>> We're already making valgrind calls here anyway.
>>
> 
> We can of cause, but we would repeat that for all other 
> _qemu_*_wrapper() functions as well. The _qemu_proc_wrapper() runs a 
> process under the Valgrind if VALGRIND_QEMU=y and the 
> _qemu_proc_valgrind_log() dumps Valgrind error reports, if any and 
> removes the Valgrind temporary log file. Again, the same pattern is used 
> as it is in the current implementation of _qemu_io_wrapper(). I have 
> just copied the functionality to all other QEMU wrapper functions.
> 
> Andrey
> 

Oh, I see now. I was just asleep at the wheel. Sorry for the hassle.
This is fine then, but maybe name it like _valgrind_exec_wrapper or some
such?

--js

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

[Andrey Shinkevich]

On 27/08/2019 22:56, John Snow wrote:
> 
> 
> On 8/25/19 11:26 AM, Andrey Shinkevich wrote:
>>
>>
>> On 16/08/2019 01:49, John Snow wrote:
>>>
>>>
>>> On 7/19/19 12:30 PM, 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
>>>> When QEMU-IO process is being killed, the shell report refers to the
>>>> text of the command in _qemu_io_wrapper(), which was modified with this
>>>> patch. So, the benchmark output for the tests 039, 061 and 137 is to be
>>>> changed also.
>>>>
>>>
>>> Oh, weird. "VALGRIND_QEMU=y" actually has just meant ... valgrind
>>> qemu-io. OK.
>>>
>>>> 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 | 69 ++++++++++++++++++++++++++++++++------------
>>>>    4 files changed, 59 insertions(+), 58 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 5502c3d..6e461a1 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
>>>> +}
>>>> +
>>>
>>> Why do we need a second wrapper? I get nervous with each new wrapper we
>>> make, because I feel like it has unintended consequences with pipe
>>> handling and so on in the test dispatcher scripts.
>>>
>>
>> The _qemu_proc_wrapper() is acctually the function rather than a new
>> wrapper. The suffix _wrapper can be changed to mislead a reader not. The
>> routine code originates from the _qemu_io_wrapper() to repeat the same
>> functionality for the QEMU processes other than qemu-io and to keep the
>> uniformity. It is also true for the function _qemu_proc_valgrind_log()
>> that dumps Valgrind reports in case of errors. The behavior is the same
>> to one in the _qemu_io_wrapper(). Actually, nothing new except that
>> _qemu_nbd_wrapper() gets the subshell exec to keep the uniformity.
>>
>> Andrey
>>
>>>> +_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 "$@"
>>>
>>> Can we not inline that logic here? especially because:
>>>
>>>>        )
>>>> +    RETVAL=$?
>>>> +    _qemu_proc_valgrind_log "${VALGRIND_LOGFILE}" $RETVAL
>>>
>>> We're already making valgrind calls here anyway.
>>>
>>
>> We can of cause, but we would repeat that for all other
>> _qemu_*_wrapper() functions as well. The _qemu_proc_wrapper() runs a
>> process under the Valgrind if VALGRIND_QEMU=y and the
>> _qemu_proc_valgrind_log() dumps Valgrind error reports, if any and
>> removes the Valgrind temporary log file. Again, the same pattern is used
>> as it is in the current implementation of _qemu_io_wrapper(). I have
>> just copied the functionality to all other QEMU wrapper functions.
>>
>> Andrey
>>
> 
> Oh, I see now. I was just asleep at the wheel. Sorry for the hassle.
> This is fine then, but maybe name it like _valgrind_exec_wrapper or some
> such?
> 
> --js
> 

In v6, I removed the word 'wrapper' and renamed the function to 
'_qemu_proc_exec'.
Message ID:
<1566834628-485525-1-git-send-email-andrey.shinkevich@virtuozzo.com>

Andrey
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [Qemu-block] [PATCH v5 4/6] iotests: Valgrind fails with nonexistent directory

[Andrey Shinkevich]

On 27/08/2019 22:45, John Snow wrote:
> 
> 
> On 8/25/19 11:24 AM, Andrey Shinkevich wrote:
>>
>>
>> On 16/08/2019 03:55, John Snow wrote:
>>>
>>>
>>> On 7/19/19 12:30 PM, 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 and notify the user of it.
>>>>
>>>> Suggested-by: Kevin Wolf <kwolf@redhat.com>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>> ---
>>>>    tests/qemu-iotests/051 | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
>>>> index ce942a5..f8141ca 100755
>>>> --- a/tests/qemu-iotests/051
>>>> +++ b/tests/qemu-iotests/051
>>>> @@ -377,6 +377,10 @@ 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
>>>> +if [ "${VALGRIND_QEMU}" == "y" ]; then
>>>> +    _casenotrun "Valgrind needs a valid TMPDIR for itself"
>>>> +fi
>>>> +VALGRIND_QEMU="" \
>>>>    TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on
>>>>    
>>>>    # Using snapshot=on together with read-only=on
>>>>
>>>
>>> The only other way around this would be a complicated mechanism to set
>>> the TMPDIR for valgrind's sub-processes only, with e.g.
>>>
>>> valgrind ... env TMPDIR=/nonexistent qemu ...
>>>
>>> ... It's probably not worth trying to concoct such a thing; but I
>>> suppose it is possible. You'd have to set up a generic layer for setting
>>> environment variables, then in the qemu shim, you could either set them
>>> directly (non-valgrind invocation) or set them as part of the valgrind
>>> command-line.
>>>
>>> Or you could just take my R-B:
>>>
>>> Reviewed-by: John Snow <jsnow@redhat.com>
>>>
>>
>> Thanks again John for your review and the advice.
>> Probably, it doesn't worth efforts to manage that case because QEMU
>> should fail anyway with the error message "Could not get temporary
>> filename: No such file or directory". So, we would not benefit much from
>> that run. We have other test cases that cover the main functionality.
>> It's just to check the QEMU error path for possible memory issues. Shall we?
>>
>> Andrey
>>
> 
> Yeah, don't bother with this for now. I just have a personal compulsion
> to try to concretely measure how much work it would take to avoid a
> "hack" and then make my decision.
> 
> You're free to just take the R-B :)
> 
> --js
> 

Thank you, John. Done in v6.
Please check the series in the email message
"[PATCH v6 0/6] Allow Valgrind checking all QEMU processes"
by 26.08.2019 with the message ID
<1566834628-485525-1-git-send-email-andrey.shinkevich@virtuozzo.com>

Andrey
-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [Qemu-block] [PATCH v5 6/6] iotests: extend sleeping time under Valgrind

[Andrey Shinkevich]

On 27/08/2019 22:42, John Snow wrote:
> 
> 
> On 8/23/19 11:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 16.08.2019 4:01, John Snow wrote:
>>>
>>>
>>> On 7/19/19 12:30 PM, Andrey Shinkevich wrote:
>>>> To synchronize the time when QEMU is running longer under the Valgrind,
>>>> increase the sleeping time in the test 247.
>>>>
>>>> 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..c853b73 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 10
>>>> +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 \
>>>>
>>>
>>> This makes me nervous, though. Won't this race terribly? (Wait, why
>>> doesn't it race already?)
>>>
>>
>> Hmm, however it works somehow. I'm afraid that everything with "sleep" is definitely racy..
>> Or what do you mean?
>>
> 
> Right -- anything with a sleep is already at risk for racing.
> 
> What I am picking up on here is that with valgrind, there is an even
> greater computational overhead that's much harder to predict, so I was
> wondering how these values were determined.
> 

I just followed the trend and extended the sleeping time with a grater 
tolerance so that the test could pass on systems where the 'sleep 1' 
command helps to pass without Valgrind. We could rewrite the test 247 in 
Python in a separate series, shall we?

Andrey

> (I wouldn't withhold an RB for that alone -- the sleeps are existing
> problems.)
> 
> What I moved on to wondering in particular is why test 247 doesn't
> already have race problems, because it looks quite fragile.
> 
> Neither of these are really Andrey's problems; I was just surprised
> momentarily that I don't see 247 fail more often already, as-is.
> 
> --js
> 

-- 
With the best regards,
Andrey Shinkevich

Re: [Qemu-devel] [Qemu-block] [PATCH v5 6/6] iotests: extend sleeping time under Valgrind

[John Snow]

On 8/28/19 11:24 AM, Andrey Shinkevich wrote:
> 
> 
> On 27/08/2019 22:42, John Snow wrote:
>>
>>
>> On 8/23/19 11:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 16.08.2019 4:01, John Snow wrote:
>>>>
>>>>
>>>> On 7/19/19 12:30 PM, Andrey Shinkevich wrote:
>>>>> To synchronize the time when QEMU is running longer under the Valgrind,
>>>>> increase the sleeping time in the test 247.
>>>>>
>>>>> 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..c853b73 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 10
>>>>> +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 \
>>>>>
>>>>
>>>> This makes me nervous, though. Won't this race terribly? (Wait, why
>>>> doesn't it race already?)
>>>>
>>>
>>> Hmm, however it works somehow. I'm afraid that everything with "sleep" is definitely racy..
>>> Or what do you mean?
>>>
>>
>> Right -- anything with a sleep is already at risk for racing.
>>
>> What I am picking up on here is that with valgrind, there is an even
>> greater computational overhead that's much harder to predict, so I was
>> wondering how these values were determined.
>>
> 
> I just followed the trend and extended the sleeping time with a grater 
> tolerance so that the test could pass on systems where the 'sleep 1' 
> command helps to pass without Valgrind. We could rewrite the test 247 in 
> Python in a separate series, shall we?
> 

If you have the time, but I don't think anyone will require it for this
series.

Just reviewing "out loud." I'll look at V6 soon.

--js