Re: [Qemu-block] [PATCH v5 4/6] iotests: Valgrind fails with nonexistent directory
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
Signed-off-by: Andrey Shinkevich
---
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
>>>
>>
>> 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.shinkev...@virtuozzo.com>
Andrey
--
With the best regards,
Andrey Shinkevich
Re: [Qemu-block] [PATCH v5 4/6] iotests: Valgrind fails with nonexistent directory
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
>>> Signed-off-by: Andrey Shinkevich
>>> ---
>>> 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
>>
>
> 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
Re: [Qemu-block] [PATCH v5 4/6] iotests: Valgrind fails with nonexistent directory
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
>> Signed-off-by: Andrey Shinkevich
>> ---
>> 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
>
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
--
With the best regards,
Andrey Shinkevich
Re: [Qemu-block] [PATCH v5 4/6] iotests: Valgrind fails with nonexistent directory
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
> Signed-off-by: Andrey Shinkevich
> ---
> 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
