| Message ID | 1563553816-148827-5-git-send-email-andrey.shinkevich@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allow Valgrind checking all QEMU processes | expand |
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>
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
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
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
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 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(+)