[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 1/6] iotests: allow Valgrind checking all QEM
From: |
Andrey Shinkevich |
Subject: |
Re: [Qemu-devel] [PATCH v6 1/6] iotests: allow Valgrind checking all QEMU processes |
Date: |
Thu, 29 Aug 2019 10:50:18 +0000 |
On 29/08/2019 03:30, Eric Blake wrote:
> On 8/28/19 5:58 PM, John Snow wrote:
>
>>> +++ b/tests/qemu-iotests/common.rc
>>> @@ -60,61 +60,132 @@ if ! . ./common.config
>>> exit 1
>>> fi
>>>
>>> +# Unset the variables to turn Valgrind off for specific processes, e.g.
>
> That's not unsetting, that's setting to the empty string.
>
Thanks Eric, I will make the correction of the comment. Any string other
than "y", including the empty one, fits.
>>> +# $ VALGRIND_QEMU_IO= ./check -qcow2 -valgrind 015
>>> +
>>> +: ${VALGRIND_QEMU_VM='y'}
>>> +: ${VALGRIND_QEMU_IMG='y'}
>>> +: ${VALGRIND_QEMU_IO='y'}
>>> +: ${VALGRIND_QEMU_NBD='y'}
>>> +: ${VALGRIND_QEMU_VXHS='y'}
>>> +
>>
I am going to make the change:
: ${VALGRIND_QEMU_VM=$VALGRIND_QEMU}
: ${VALGRIND_QEMU_IMG=$VALGRIND_QEMU}
: ${VALGRIND_QEMU_IO=$VALGRIND_QEMU}
: ${VALGRIND_QEMU_NBD=$VALGRIND_QEMU}
: ${VALGRIND_QEMU_VXHS=$VALGRIND_QEMU}
and get rid of the local VALGRIND_ON="${VALGRIND_QEMU}"
so that the code will be optimized.
>> I have to admit to you that I'm not familiar with this trick. I'm
>> looking it up and I see := documented, but not = alone.
>
> It's been a repeated complaint to the bash developer that the manual is
> doing a disservice to its users by not documenting ${var=val} in an
> easily searchable form. It IS documented, but only by virtue of
> ${var:=val} occurring under a section header that states:
>
> When not performing substring expansion, using the forms
> documented
> below (e.g., :-), bash tests for a parameter that is unset or
> null.
> Omitting the colon results in a test only for a parameter
> that is
> unset.
>
> So the choice is whether you want to special case a variable set to an
> empty string the same as an unset variable, or the same as a variable
> with a non-empty value.
>
Thank you all for your reviews and comments. The purpose why I omitted
the colon is to allow a user writing the shorter command syntax like
$ VALGRIND_QEMU_IO= ./check -valgrind <test#>
rather than
$ VALGRIND_QEMU_IO=" 'no' or 'off' or else anything other than 'y' "
./check -valgrind <test#>
so, no need to strike the Shift key twice and guess at what else is
acceptable to type )))
The variable default value 'y' looks good to me to implement the new
functionality that is compatible with the existing one when we just set
the '-valgrind' switch. The general idea behind using the Valgrind is to
make a careful search for memory issues. Once found, a user can tune the
particular test with extra variables to save their development/testing
time as John suggested. Also, no need to specify all the five long name
variables each time a user writes the command if default values aren't set.
I am flexible to make a change that is good for all. So, what solution
will we come to?
Andrey
>>
>> It doesn't seem documented here at all:
>> https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html
>>
>> I see it here, though:
>> https://www.tldp.org/LDP/abs/html/parameter-substitution.html
>>
>> And it seems to work, but I'm not sure if this works with BSD or OSX's
>> sh. I see Eric comment on that compatibility a lot, so maybe I'll let
>> him chime in.
>
> It's quite portable; POSIX requires it, and autoconf relies on it.
>
--
With the best regards,
Andrey Shinkevich
- [Qemu-devel] [PATCH v6 3/6] iotests: Add casenotrun report to bash tests, (continued)
- [Qemu-devel] [PATCH v6 3/6] iotests: Add casenotrun report to bash tests, Andrey Shinkevich, 2019/08/26
- [Qemu-devel] [PATCH v6 2/6] iotests: exclude killed processes from running under Valgrind, Andrey Shinkevich, 2019/08/26
- [Qemu-devel] [PATCH v6 6/6] iotests: extend sleeping time under Valgrind, Andrey Shinkevich, 2019/08/26
- [Qemu-devel] [PATCH v6 5/6] iotests: extended timeout under Valgrind, Andrey Shinkevich, 2019/08/26
- [Qemu-devel] [PATCH v6 4/6] iotests: Valgrind fails with nonexistent directory, Andrey Shinkevich, 2019/08/26
- [Qemu-devel] [PATCH v6 1/6] iotests: allow Valgrind checking all QEMU processes, Andrey Shinkevich, 2019/08/26