Since we switched to using parallel tests, our valgrind tests would definitely have found leaks in the /usr/bin/env program if it had any. Not so much in some of our own programs. Let's fix that. Rich.
Richard W.M. Jones
2016-Feb-22 13:59 UTC
[Libguestfs] [PATCH 1/8] run.in: Remove out of date documentation for using $(VG).
Since we enabled parallel tests, you can no longer run tests
under valgrind merely by doing:
TESTS_ENVIRONMENT = $(top_builddir)/run --test $(VG)
check-valgrind:
$(MAKE) check VG="@VG@"
The reason is that the parallel tests framework doesn't run
``$(TESTS_ENVIRONMENT) <test>''. It inserts some other processes
in
between the environment and the test, so you end up valgrinding some
unrelated process (currently ``env'').
---
run.in | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/run.in b/run.in
index ed4109c..c911834 100755
--- a/run.in
+++ b/run.in
@@ -33,13 +33,10 @@
#
# The script should also be used for tests like this:
#
-# TESTS_ENVIRONMENT = ... $(top_builddir)/run --test [$(VG)]
+# TESTS_ENVIRONMENT = ... $(top_builddir)/run --test
#
# The --test parameter introduces a timeout, stopping tests from
# running forever.
-#
-# Use the optional $(VG) when the tests must also be run under
-# valgrind.
#----------------------------------------------------------------------
--
2.5.0
Richard W.M. Jones
2016-Feb-22 13:59 UTC
[Libguestfs] [PATCH 2/8] docs: Describe how to set up check-valgrind rules correctly.
--- docs/guestfs-hacking.pod | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/docs/guestfs-hacking.pod b/docs/guestfs-hacking.pod index c357717..419a4c2 100644 --- a/docs/guestfs-hacking.pod +++ b/docs/guestfs-hacking.pod @@ -274,8 +274,7 @@ the automake documentation for details. Runs a subset of the test suite under valgrind. -Any F<Makefile.am> in the tree that has a C<check-valgrind:> target -will be run by this rule. +See L</VALGRIND> below. =item C<make check-valgrind-local-guests> @@ -383,6 +382,35 @@ Do: =back +=head2 VALGRIND + +When you do C<make check-valgrind>, it searches for any F<Makefile.am> +in the tree that has a C<check-valgrind:> target and runs it. + +Writing the F<Makefile.am> and tests correctly to use valgrind and +working with automake parallel tests is subtle. + +If your tests are run via a shell script wrapper, then in the wrapper +use: + + $VG virt-foo + +and in the F<Makefile.am> use: + + check-valgrind: + make VG="$(top_builddir)/run @VG@" check + +However, if your binaries run directly from the C<TESTS> rule, you +have to modify the F<Makefile.am> like this: + + LOG_COMPILER = $(VG) + + check-valgrind: + make VG="@VG@" check + +In either case, check that the right program is being tested by +examining the F<tmp/valgrind*> log files carefully. + =head1 SUBMITTING PATCHES Submit patches to the mailing list: -- 2.5.0
Richard W.M. Jones
2016-Feb-22 13:59 UTC
[Libguestfs] [PATCH 3/8] tests: daemon: Remove bogus use of $(VG).
--- tests/daemon/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/daemon/Makefile.am b/tests/daemon/Makefile.am index bb380c5..053cad3 100644 --- a/tests/daemon/Makefile.am +++ b/tests/daemon/Makefile.am @@ -27,7 +27,7 @@ TESTS = \ test-daemon-start.pl \ test-btrfs.pl -TESTS_ENVIRONMENT = $(top_builddir)/run --test $(VG) +TESTS_ENVIRONMENT = $(top_builddir)/run --test EXTRA_DIST = \ captive-daemon.pm \ -- 2.5.0
Richard W.M. Jones
2016-Feb-22 13:59 UTC
[Libguestfs] [PATCH 4/8] lua: Remove bogus use of $(VG).
No check-valgrind is used in this subdirectory anyway. --- lua/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lua/Makefile.am b/lua/Makefile.am index 4c01e80..264f170 100644 --- a/lua/Makefile.am +++ b/lua/Makefile.am @@ -64,7 +64,7 @@ guestfs.so: libluaguestfs.la ln -sf .libs/libluaguestfs.so $@ # Tests. -TESTS_ENVIRONMENT = LC_ALL=C $(top_builddir)/run --test $(VG) +TESTS_ENVIRONMENT = LC_ALL=C $(top_builddir)/run --test TESTS = \ run-bindtests \ tests/010-load.lua \ -- 2.5.0
Richard W.M. Jones
2016-Feb-22 13:59 UTC
[Libguestfs] [PATCH 5/8] ocaml: Use LOG_COMPILER to run valgrind.
--- ocaml/Makefile.am | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ocaml/Makefile.am b/ocaml/Makefile.am index bc1d130..2152312 100644 --- a/ocaml/Makefile.am +++ b/ocaml/Makefile.am @@ -105,7 +105,8 @@ clean-local: endif -TESTS_ENVIRONMENT = $(top_builddir)/run --test $(VG) +TESTS_ENVIRONMENT = $(top_builddir)/run --test +LOG_COMPILER = $(VG) test_progs_bc = \ t/guestfs_010_load.bc \ @@ -164,7 +165,7 @@ if HAVE_OCAMLOPT endif check-valgrind: - $(MAKE) VG="$(top_builddir)/run @VG@" TESTS="$(test_progs_all)" check + $(MAKE) VG="@VG@" TESTS="$(test_progs_all)" check CLEANFILES += bindtests.tmp -- 2.5.0
Richard W.M. Jones
2016-Feb-22 13:59 UTC
[Libguestfs] [PATCH 6/8] lib: Use LOG_COMPILER to run valgrind.
--- src/Makefile.am | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Makefile.am b/src/Makefile.am index 25c6fa3..3ebb7f5 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -253,7 +253,8 @@ endif # internal tests of utility functions. Note we don't just test what's # in utils.c, we can test other functions as well here. -TESTS_ENVIRONMENT = $(top_builddir)/run --test $(VG) +TESTS_ENVIRONMENT = $(top_builddir)/run --test +LOG_COMPILER = $(VG) TESTS = test-utils check_PROGRAMS = test-utils -- 2.5.0
Richard W.M. Jones
2016-Feb-22 13:59 UTC
[Libguestfs] [PATCH 7/8] tests: c-api: Use LOG_COMPILER to run valgrind.
--- tests/c-api/Makefile.am | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/c-api/Makefile.am b/tests/c-api/Makefile.am index 6503d94..40fe33e 100644 --- a/tests/c-api/Makefile.am +++ b/tests/c-api/Makefile.am @@ -80,7 +80,8 @@ TESTS_ENVIRONMENT = \ SKIP_TEST_COMMAND=$(shell ldd test-command | grep -sq 'not a dynamic executable' || echo 1) \ SKIP_TEST_COMMAND_LINES=$(shell ldd test-command | grep -sq 'not a dynamic executable' || echo 1) \ SKIP_TEST_COMMAND=$(shell ldd test-pwd | grep -sq 'not a dynamic executable' || echo 1) \ - $(top_builddir)/run --test $(VG) + $(top_builddir)/run --test +LOG_COMPILER = $(VG) #SKIP_TEST_CHECKSUM_8=$(shell if test `find ../initramfs -name squashfs.ko | wc -l` -eq 0; then echo 1; fi) @@ -258,4 +259,4 @@ test_add_libvirt_dom_LDADD = \ endif check-valgrind: - $(MAKE) VG="$(top_builddir)/run @VG@" check + $(MAKE) VG="@VG@" check -- 2.5.0
Richard W.M. Jones
2016-Feb-22 13:59 UTC
[Libguestfs] [PATCH 8/8] tests: mount-local: Use LOG_COMPILER to run valgrind.
--- tests/mount-local/Makefile.am | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/mount-local/Makefile.am b/tests/mount-local/Makefile.am index eac2cc8..b3c86cf 100644 --- a/tests/mount-local/Makefile.am +++ b/tests/mount-local/Makefile.am @@ -21,7 +21,8 @@ if HAVE_FUSE TESTS = test-parallel-mount-local -TESTS_ENVIRONMENT = $(top_builddir)/run --test $(VG) +TESTS_ENVIRONMENT = $(top_builddir)/run --test +LOG_COMPILER = $(VG) check_PROGRAMS = $(TESTS) @@ -48,6 +49,6 @@ test_parallel_mount_local_LDADD = \ $(top_builddir)/gnulib/lib/libgnu.la check-valgrind: - $(MAKE) VG="$(top_builddir)/run @VG@" check + $(MAKE) VG="@VG@" check endif -- 2.5.0
On Monday 22 February 2016 13:59:11 Richard W.M. Jones wrote:> Since we switched to using parallel tests, our valgrind tests would > definitely have found leaks in the /usr/bin/env program if it had any. > Not so much in some of our own programs. Let's fix that.The series LGTM, but do they make sense as separate patches? Most probably a single one conveying the change across directories would be better (even from a debugging/bisecting PoV). Thanks, -- Pino Toscano
On Mon, Feb 22, 2016 at 06:49:47PM +0100, Pino Toscano wrote:> On Monday 22 February 2016 13:59:11 Richard W.M. Jones wrote: > > Since we switched to using parallel tests, our valgrind tests would > > definitely have found leaks in the /usr/bin/env program if it had any. > > Not so much in some of our own programs. Let's fix that. > > The series LGTM, but do they make sense as separate patches? Most > probably a single one conveying the change across directories would > be better (even from a debugging/bisecting PoV).Yup, I'll squash 'em a bit. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v