diff mbox series

[v2,7/7] iotests: new file to suppress Valgrind errors

Message ID 1560276131-683243-8-git-send-email-andrey.shinkevich@virtuozzo.com
State New
Headers show
Series Allow Valgrind checking all QEMU processes | expand

Commit Message

Andrey Shinkevich June 11, 2019, 6:02 p.m. UTC
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

Comments

Vladimir Sementsov-Ogievskiy June 13, 2019, 10:06 a.m. UTC | #1
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
> +}
>
Kevin Wolf June 17, 2019, 11:45 a.m. UTC | #2
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
Andrey Shinkevich June 24, 2019, 4:55 p.m. UTC | #3
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
>
Andrey Shinkevich June 24, 2019, 4:55 p.m. UTC | #4
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
>> +}
>>
> 
>
Eric Blake June 24, 2019, 5:09 p.m. UTC | #5
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.
Andrey Shinkevich June 24, 2019, 5:23 p.m. UTC | #6
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
Kevin Wolf June 25, 2019, 8:13 a.m. UTC | #7
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 mbox series

Patch

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