From: John Snow <jsnow@redhat.com> To: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org Cc: kwolf@redhat.com, den@openvz.org, vsementsov@virtuozzo.com, mreitz@redhat.com Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v5 1/6] iotests: allow Valgrind checking all QEMU processes Date: Thu, 15 Aug 2019 18:49:18 -0400 Message-ID: <e1448863-8133-dec0-bce3-7b13664deb6b@redhat.com> (raw) In-Reply-To: <1563553816-148827-2-git-send-email-andrey.shinkevich@virtuozzo.com> 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.)
next prev parent reply index Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-19 16:30 [Qemu-devel] [PATCH v5 0/6] Allow " Andrey Shinkevich 2019-07-19 16:30 ` [Qemu-devel] [PATCH v5 1/6] iotests: allow " Andrey Shinkevich 2019-08-15 22:49 ` John Snow [this message] 2019-08-25 15:26 ` [Qemu-devel] [Qemu-block] " Andrey Shinkevich 2019-08-27 19:56 ` John Snow 2019-08-28 15:04 ` Andrey Shinkevich 2019-07-19 16:30 ` [Qemu-devel] [PATCH v5 2/6] iotests: exclude killed processes from running under Valgrind Andrey Shinkevich 2019-08-16 0:40 ` [Qemu-devel] [Qemu-block] " John Snow 2019-07-19 16:30 ` [Qemu-devel] [PATCH v5 3/6] iotests: Add casenotrun report to bash tests Andrey Shinkevich 2019-08-16 0:44 ` [Qemu-devel] [Qemu-block] " John Snow 2019-08-16 20:33 ` Cleber Rosa 2019-08-25 13:03 ` Andrey Shinkevich 2019-07-19 16:30 ` [Qemu-devel] [PATCH v5 4/6] iotests: Valgrind fails with nonexistent directory Andrey Shinkevich 2019-08-16 0:55 ` [Qemu-devel] [Qemu-block] " John Snow 2019-08-25 15:24 ` Andrey Shinkevich 2019-08-27 19:45 ` John Snow 2019-08-28 15:12 ` Andrey Shinkevich 2019-07-19 16:30 ` [Qemu-devel] [PATCH v5 5/6] iotests: extended timeout under Valgrind Andrey Shinkevich 2019-08-16 0:58 ` [Qemu-devel] [Qemu-block] " John Snow 2019-07-19 16:30 ` [Qemu-devel] [PATCH v5 6/6] iotests: extend sleeping time " Andrey Shinkevich 2019-08-16 1:01 ` [Qemu-devel] [Qemu-block] " John Snow 2019-08-23 15:27 ` Vladimir Sementsov-Ogievskiy 2019-08-27 19:42 ` John Snow 2019-08-28 15:24 ` Andrey Shinkevich 2019-08-28 17:27 ` John Snow 2019-08-25 10:13 ` Andrey Shinkevich 2019-08-06 16:24 ` [Qemu-devel] [PATCH v5 0/6] Allow Valgrind checking all QEMU processes Andrey Shinkevich 2019-08-16 20:05 ` Cleber Rosa 2019-08-25 10:30 ` Andrey Shinkevich
Reply instructions: You may reply publically to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=e1448863-8133-dec0-bce3-7b13664deb6b@redhat.com \ --to=jsnow@redhat.com \ --cc=andrey.shinkevich@virtuozzo.com \ --cc=den@openvz.org \ --cc=kwolf@redhat.com \ --cc=mreitz@redhat.com \ --cc=qemu-block@nongnu.org \ --cc=qemu-devel@nongnu.org \ --cc=vsementsov@virtuozzo.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
QEMU-Devel Archive on lore.kernel.org Archives are clonable: git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \ qemu-devel@nongnu.org public-inbox-index qemu-devel Example config snippet for mirrors Newsgroup available over NNTP: nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel AGPL code for this site: git clone https://public-inbox.org/public-inbox.git