| Message ID | 1560276131-683243-8-git-send-email-andrey.shinkevich@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | Allow Valgrind checking all QEMU processes | expand |
11.06.2019 21:02, Andrey Shinkevich wrote: > The Valgrind tool reports about an uninitialised memory usage when the > initialization is actually not needed. For example, the buffer 'buf' > instantiated on a stack of the function guess_disk_lchs(). for convinience, you may add: "of the function guess_disk_lchs(), which is then actually initialized by blk_pread_unthrottled()" > Let's use the Valgrind technology to suppress the unwanted reports by > adding the Valgrind specific format file valgrind.supp to the QEMU > project. The file content is extendable for future needs. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > --- > tests/qemu-iotests/common.rc | 5 ++++- > tests/qemu-iotests/valgrind.supp | 31 +++++++++++++++++++++++++++++++ > 2 files changed, 35 insertions(+), 1 deletion(-) > create mode 100644 tests/qemu-iotests/valgrind.supp > > diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc > index 3caaca4..9b74890 100644 > --- a/tests/qemu-iotests/common.rc > +++ b/tests/qemu-iotests/common.rc > @@ -17,6 +17,8 @@ > # along with this program. If not, see <http://www.gnu.org/licenses/>. > # > > +readonly VALGRIND_SUPPRESS_ERRORS=./valgrind.supp Why readonly? I think it should be defined near and in same manner as VALGRIND_LOGFILE, with use of TEST_DIR > + > SED= > for sed in sed gsed; do > ($sed --version | grep 'GNU sed') > /dev/null 2>&1 > @@ -65,7 +67,8 @@ _qemu_proc_wrapper() > local VALGRIND_LOGFILE="$1" > shift > if [ "${VALGRIND_QEMU}" == "y" ]; then > - exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$@" > + exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 \ > + --suppressions="${VALGRIND_SUPPRESS_ERRORS}" "$@" > else > exec "$@" > fi > diff --git a/tests/qemu-iotests/valgrind.supp b/tests/qemu-iotests/valgrind.supp > new file mode 100644 > index 0000000..78215b6 > --- /dev/null > +++ b/tests/qemu-iotests/valgrind.supp > @@ -0,0 +1,31 @@ > +# > +# Valgrind errors suppression file for QEMU iotests > +# > +# Copyright (c) 2019 Virtuozzo International GmbH > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 2 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > +# > +{ > + hw/block/hd-geometry.c > + Memcheck:Cond > + fun:guess_disk_lchs > + fun:hd_geometry_guess > + fun:blkconf_geometry > + ... > + fun:device_set_realized > + fun:property_set_bool > + fun:object_property_set > + fun:object_property_set_qobject > + fun:object_property_set_bool > +} >
Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben: > The Valgrind tool reports about an uninitialised memory usage when the > initialization is actually not needed. For example, the buffer 'buf' > instantiated on a stack of the function guess_disk_lchs(). I would be careful with calling initialisation "not needed". It means that the test case may not behave entirely determinstic because the uninitialised memory can vary between runs. In this specific case, I assume that guess_disk_lchs() is called for a null block node, for which .bdrv_co_preadv by default returns without actually writing to the buffer. Instead of ignoring the valgrind error, we could instead pass read-zeroes=on to the null block driver to make the test deterministic. (Unfortunately, while adding the read-zeroes option, we didn't add it to the QAPI schema, so it's not available yet in -blockdev. I'm going to send a fix for that, but most of the problematic test cases probably don't even use -blockdev.) Kevin
On 17/06/2019 14:45, Kevin Wolf wrote: > Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben: >> The Valgrind tool reports about an uninitialised memory usage when the >> initialization is actually not needed. For example, the buffer 'buf' >> instantiated on a stack of the function guess_disk_lchs(). > > I would be careful with calling initialisation "not needed". It means > that the test case may not behave entirely determinstic because the > uninitialised memory can vary between runs.\ I am going to amend the comment. Andrey > > In this specific case, I assume that guess_disk_lchs() is called for a > null block node, for which .bdrv_co_preadv by default returns without > actually writing to the buffer. Instead of ignoring the valgrind error, > we could instead pass read-zeroes=on to the null block driver to make > the test deterministic. The buffer that the Valgrind complains of is initialized by the following function call blk_pread_unthrottled() that reads the first BDRV_SECTOR_SIZE bytes form a disk "to guess the disk logical geometry". The Valgrind does not recognize that way of initialization. I believe we do not need to zero the buffer instantiated on the stack just to make the Valgrind silent there. Andrey > > (Unfortunately, while adding the read-zeroes option, we didn't add it to > the QAPI schema, so it's not available yet in -blockdev. I'm going to > send a fix for that, but most of the problematic test cases probably > don't even use -blockdev.) > > Kevin >
On 13/06/2019 13:06, Vladimir Sementsov-Ogievskiy wrote: > 11.06.2019 21:02, Andrey Shinkevich wrote: >> The Valgrind tool reports about an uninitialised memory usage when the >> initialization is actually not needed. For example, the buffer 'buf' >> instantiated on a stack of the function guess_disk_lchs(). > > for convinience, you may add: "of the function guess_disk_lchs(), which > is then actually initialized by blk_pread_unthrottled()" > >> Let's use the Valgrind technology to suppress the unwanted reports by >> adding the Valgrind specific format file valgrind.supp to the QEMU >> project. The file content is extendable for future needs. >> >> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> >> --- >> tests/qemu-iotests/common.rc | 5 ++++- >> tests/qemu-iotests/valgrind.supp | 31 +++++++++++++++++++++++++++++++ >> 2 files changed, 35 insertions(+), 1 deletion(-) >> create mode 100644 tests/qemu-iotests/valgrind.supp >> >> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc >> index 3caaca4..9b74890 100644 >> --- a/tests/qemu-iotests/common.rc >> +++ b/tests/qemu-iotests/common.rc >> @@ -17,6 +17,8 @@ >> # along with this program. If not, see <http://www.gnu.org/licenses/>. >> # >> >> +readonly VALGRIND_SUPPRESS_ERRORS=./valgrind.supp > > Why readonly? > > I think it should be defined near and in same manner as VALGRIND_LOGFILE, > with use of TEST_DIR > The new file 'valgrind.supp' is intended to be a part of the QEMU project. So, it will be located in the test directory tests/qemu-iotests. The variable TEST_DIR may change the working directory. In that case, moving the project file will be a hassle. Andrey >> + >> SED= >> for sed in sed gsed; do >> ($sed --version | grep 'GNU sed') > /dev/null 2>&1 >> @@ -65,7 +67,8 @@ _qemu_proc_wrapper() >> local VALGRIND_LOGFILE="$1" >> shift >> if [ "${VALGRIND_QEMU}" == "y" ]; then >> - exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$@" >> + exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 \ >> + --suppressions="${VALGRIND_SUPPRESS_ERRORS}" "$@" >> else >> exec "$@" >> fi >> diff --git a/tests/qemu-iotests/valgrind.supp b/tests/qemu-iotests/valgrind.supp >> new file mode 100644 >> index 0000000..78215b6 >> --- /dev/null >> +++ b/tests/qemu-iotests/valgrind.supp >> @@ -0,0 +1,31 @@ >> +# >> +# Valgrind errors suppression file for QEMU iotests >> +# >> +# Copyright (c) 2019 Virtuozzo International GmbH >> +# >> +# This program is free software; you can redistribute it and/or modify >> +# it under the terms of the GNU General Public License as published by >> +# the Free Software Foundation; either version 2 of the License, or >> +# (at your option) any later version. >> +# >> +# This program is distributed in the hope that it will be useful, >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> +# GNU General Public License for more details. >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program. If not, see <http://www.gnu.org/licenses/>. >> +# >> +{ >> + hw/block/hd-geometry.c >> + Memcheck:Cond >> + fun:guess_disk_lchs >> + fun:hd_geometry_guess >> + fun:blkconf_geometry >> + ... >> + fun:device_set_realized >> + fun:property_set_bool >> + fun:object_property_set >> + fun:object_property_set_qobject >> + fun:object_property_set_bool >> +} >> > >
On 6/24/19 11:55 AM, Andrey Shinkevich wrote: >>> +++ b/tests/qemu-iotests/common.rc >>> @@ -17,6 +17,8 @@ >>> # along with this program. If not, see <http://www.gnu.org/licenses/>. >>> # >>> >>> +readonly VALGRIND_SUPPRESS_ERRORS=./valgrind.supp >> >> Why readonly? >> >> I think it should be defined near and in same manner as VALGRIND_LOGFILE, >> with use of TEST_DIR >> > > The new file 'valgrind.supp' is intended to be a part of the QEMU > project. So, it will be located in the test directory tests/qemu-iotests. > The variable TEST_DIR may change the working directory. In that case, > moving the project file will be a hassle. My personal thoughts are that no serious POSIX or bash shell script ever uses readonly (it offers the illusion of security but cannot actually back it up, and in reality ends up causing more bugs than it prevents when your script breaks because you tried to modify a readonly variable). I've only ever dealt with one project that tried to use readonly in earnest (the 'cygport' script for building Cygwin packages) and it got in the way more than it saved me from bugs. Declaring that VALGRIND_SUPPRESS_ERRORS is initialized hard-coded to ./ instead of relative to ${TEST_DIR} is orthogonal to whether you declare that the variable VALGRIND_SUPPRESS_ERRORS can no longer be further modified by the rest of the script.
On 24/06/2019 20:09, Eric Blake wrote: > On 6/24/19 11:55 AM, Andrey Shinkevich wrote: > >>>> +++ b/tests/qemu-iotests/common.rc >>>> @@ -17,6 +17,8 @@ >>>> # along with this program. If not, see <http://www.gnu.org/licenses/>. >>>> # >>>> >>>> +readonly VALGRIND_SUPPRESS_ERRORS=./valgrind.supp >>> >>> Why readonly? >>> >>> I think it should be defined near and in same manner as VALGRIND_LOGFILE, >>> with use of TEST_DIR >>> >> >> The new file 'valgrind.supp' is intended to be a part of the QEMU >> project. So, it will be located in the test directory tests/qemu-iotests. >> The variable TEST_DIR may change the working directory. In that case, >> moving the project file will be a hassle. > > My personal thoughts are that no serious POSIX or bash shell script ever > uses readonly (it offers the illusion of security but cannot actually > back it up, and in reality ends up causing more bugs than it prevents > when your script breaks because you tried to modify a readonly > variable). I've only ever dealt with one project that tried to use > readonly in earnest (the 'cygport' script for building Cygwin packages) > and it got in the way more than it saved me from bugs. > > Declaring that VALGRIND_SUPPRESS_ERRORS is initialized hard-coded to ./ > instead of relative to ${TEST_DIR} is orthogonal to whether you declare > that the variable VALGRIND_SUPPRESS_ERRORS can no longer be further > modified by the rest of the script. > Thank you Eric. I am flexible on that subject. If the path is relative to ${TEST_DIR}, should the file valgrind.supp be copied from ./ to the ${TEST_DIR} by the script itself? Andrey
Am 24.06.2019 um 18:55 hat Andrey Shinkevich geschrieben: > > > On 17/06/2019 14:45, Kevin Wolf wrote: > > Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben: > >> The Valgrind tool reports about an uninitialised memory usage when the > >> initialization is actually not needed. For example, the buffer 'buf' > >> instantiated on a stack of the function guess_disk_lchs(). > > > > I would be careful with calling initialisation "not needed". It means > > that the test case may not behave entirely determinstic because the > > uninitialised memory can vary between runs.\ > > I am going to amend the comment. > > Andrey > > > > > In this specific case, I assume that guess_disk_lchs() is called for a > > null block node, for which .bdrv_co_preadv by default returns without > > actually writing to the buffer. Instead of ignoring the valgrind error, > > we could instead pass read-zeroes=on to the null block driver to make > > the test deterministic. > > The buffer that the Valgrind complains of is initialized by the > following function call blk_pread_unthrottled() that reads the first > BDRV_SECTOR_SIZE bytes form a disk "to guess the disk logical geometry". > The Valgrind does not recognize that way of initialization. I believe we > do not need to zero the buffer instantiated on the stack just to make > the Valgrind silent there. My point is that blk_pread_unthrottled() with null-co/null-aio leaves the buffer untouched if read-zeroes=off (which is the default). So yes, valgrind is right, this memory is still uninitialised after blk_pread_unthrottled(). Kevin
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc index 3caaca4..9b74890 100644 --- a/tests/qemu-iotests/common.rc +++ b/tests/qemu-iotests/common.rc @@ -17,6 +17,8 @@ # along with this program. If not, see <http://www.gnu.org/licenses/>. # +readonly VALGRIND_SUPPRESS_ERRORS=./valgrind.supp + SED= for sed in sed gsed; do ($sed --version | grep 'GNU sed') > /dev/null 2>&1 @@ -65,7 +67,8 @@ _qemu_proc_wrapper() local VALGRIND_LOGFILE="$1" shift if [ "${VALGRIND_QEMU}" == "y" ]; then - exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 "$@" + exec valgrind --log-file="${VALGRIND_LOGFILE}" --error-exitcode=99 \ + --suppressions="${VALGRIND_SUPPRESS_ERRORS}" "$@" else exec "$@" fi diff --git a/tests/qemu-iotests/valgrind.supp b/tests/qemu-iotests/valgrind.supp new file mode 100644 index 0000000..78215b6 --- /dev/null +++ b/tests/qemu-iotests/valgrind.supp @@ -0,0 +1,31 @@ +# +# Valgrind errors suppression file for QEMU iotests +# +# Copyright (c) 2019 Virtuozzo International GmbH +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# +{ + hw/block/hd-geometry.c + Memcheck:Cond + fun:guess_disk_lchs + fun:hd_geometry_guess + fun:blkconf_geometry + ... + fun:device_set_realized + fun:property_set_bool + fun:object_property_set + fun:object_property_set_qobject + fun:object_property_set_bool +}
The Valgrind tool reports about an uninitialised memory usage when the initialization is actually not needed. For example, the buffer 'buf' instantiated on a stack of the function guess_disk_lchs(). Let's use the Valgrind technology to suppress the unwanted reports by adding the Valgrind specific format file valgrind.supp to the QEMU project. The file content is extendable for future needs. Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- tests/qemu-iotests/common.rc | 5 ++++- tests/qemu-iotests/valgrind.supp | 31 +++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 tests/qemu-iotests/valgrind.supp