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 <kw...@redhat.com> >>>> Signed-off-by: Andrey Shinkevich <andrey.shinkev...@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 <js...@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.shinkev...@virtuozzo.com> Andrey -- With the best regards, Andrey Shinkevich