Re: [Qemu-devel] [Qemu-block] [PATCH v5 4/6] iotests: Valgrind fails with nonexistent directory

[John Snow]

On 8/25/19 11:24 AM, Andrey Shinkevich wrote:
> 
> 
> On 16/08/2019 03:55, John Snow wrote:
>>
>>
>> On 7/19/19 12:30 PM, Andrey Shinkevich wrote:
>>> The Valgrind uses the exported variable TMPDIR and fails if the
>>> directory does not exist. Let us exclude such a test case from
>>> being run under the Valgrind and notify the user of it.
>>>
>>> Suggested-by: Kevin Wolf <address@hidden>
>>> Signed-off-by: Andrey Shinkevich <address@hidden>
>>> ---
>>>   tests/qemu-iotests/051 | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
>>> index ce942a5..f8141ca 100755
>>> --- a/tests/qemu-iotests/051
>>> +++ b/tests/qemu-iotests/051
>>> @@ -377,6 +377,10 @@ printf %b "qemu-io $device_id \"write -P 0x33 0 
>>> 4k\"\ncommit $device_id\n" |
>>>   $QEMU_IO -c "read -P 0x33 0 4k" "$TEST_IMG" | _filter_qemu_io
>>>   
>>>   # Using snapshot=on with a non-existent TMPDIR
>>> +if [ "${VALGRIND_QEMU}" == "y" ]; then
>>> +    _casenotrun "Valgrind needs a valid TMPDIR for itself"
>>> +fi
>>> +VALGRIND_QEMU="" \
>>>   TMPDIR=/nonexistent run_qemu -drive driver=null-co,snapshot=on
>>>   
>>>   # Using snapshot=on together with read-only=on
>>>
>>
>> The only other way around this would be a complicated mechanism to set
>> the TMPDIR for valgrind's sub-processes only, with e.g.
>>
>> valgrind ... env TMPDIR=/nonexistent qemu ...
>>
>> ... It's probably not worth trying to concoct such a thing; but I
>> suppose it is possible. You'd have to set up a generic layer for setting
>> environment variables, then in the qemu shim, you could either set them
>> directly (non-valgrind invocation) or set them as part of the valgrind
>> command-line.
>>
>> Or you could just take my R-B:
>>
>> Reviewed-by: John Snow <address@hidden>
>>
> 
> Thanks again John for your review and the advice.
> Probably, it doesn't worth efforts to manage that case because QEMU 
> should fail anyway with the error message "Could not get temporary 
> filename: No such file or directory". So, we would not benefit much from 
> that run. We have other test cases that cover the main functionality. 
> It's just to check the QEMU error path for possible memory issues. Shall we?
> 
> Andrey
> 

Yeah, don't bother with this for now. I just have a personal compulsion
to try to concretely measure how much work it would take to avoid a
"hack" and then make my decision.

You're free to just take the R-B :)

--js