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

[Eric Blake]

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

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

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

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

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

signature.asc
Description: OpenPGP digital signature