[v5,4/6] iotests: Valgrind fails with nonexistent directory
diff mbox series

Message ID 1563553816-148827-5-git-send-email-andrey.shinkevich@virtuozzo.com
State New
Headers show
Series
  • Allow Valgrind checking all QEMU processes
Related show

Commit Message

Andrey Shinkevich July 19, 2019, 4:30 p.m. UTC
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 <kwolf@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/051 | 4 ++++
 1 file changed, 4 insertions(+)

Comments

John Snow Aug. 16, 2019, 12:55 a.m. UTC | #1
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 <kwolf@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>  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 <jsnow@redhat.com>
Andrey Shinkevich Aug. 25, 2019, 3:24 p.m. UTC | #2
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 <kwolf@redhat.com>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> ---
>>   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 <jsnow@redhat.com>
> 

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
John Snow Aug. 27, 2019, 7:45 p.m. UTC | #3
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 <kwolf@redhat.com>
>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>> ---
>>>   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 <jsnow@redhat.com>
>>
> 
> 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
Andrey Shinkevich Aug. 28, 2019, 3:12 p.m. UTC | #4
On 27/08/2019 22:45, John Snow wrote:
> 
> 
> 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 <kwolf@redhat.com>
>>>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>>>> ---
>>>>    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 <jsnow@redhat.com>
>>>
>>
>> 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
> 

Thank you, John. Done in v6.
Please check the series in the email message
"[PATCH v6 0/6] Allow Valgrind checking all QEMU processes"
by 26.08.2019 with the message ID
<1566834628-485525-1-git-send-email-andrey.shinkevich@virtuozzo.com>

Andrey

Patch
diff mbox series

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