Bug 24258

Branch updated, rebased onto David's last commit, "Merge branch 'tests' to fix fd.o #25384". I've also split it up a bit, and put it in priority order, for ease of review.

The first patch is a real reference leak, albeit to the globally-shared starter bus TpDBusDaemon. It doesn't affect 0.8.x, which doesn't have that code.

The rest of the patches up to and including 'leaks' probably affect 0.8.x, but are only in tests and examples. They should be backported into 0.8.x if approved (it's likely to be trivial to do so).

The rest of the patches up to and including 'valgrind' annotate intentional one-per-process leaks, and improve our Valgrind setup. The idea is that this will make valgrinding CMs, MC, etc. more useful too.

Looks good up to leaks

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=94dae71c582130aa053c953311b604b6d8f04e28 which required me to look both at the commit message and the full source to see what was going on. Would having a VALGRIND_TESTS variable make it clearer?

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=7c0af0be354edad11930d2d0e6de897ae7a57253
If we want to have a test that checks whether valgrind likes our program, wouldn't it be better to have it fail if valgrind doesn't like us? Are the suppressions in later commits enough to make this pass?

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=63ceaa9286a5b5ba1cf30b9954601140b6b7893d looks good if my guess at what the ... syntax means is correct. I guess you tested it and it did what you wanted, and it makes the code simpler, which is more important.

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=63ceaa9286a5b5ba1cf30b9954601140b6b7893d -- Have you made any changes that could/should be published to http://live.gnome.org/Valgrind?

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=8f69846e0eacf758f35eb3d8d1f6034e243e6bb2 -- I like these kinds of commits.

http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=2fa0d932088ca4d8812bbf3f46f787d51ff94730 http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=2d11ca85e44f33f590b352559c3b8016d7673945 -- Would it be possible to make our tests drop off the bus before exiting, or is this too much work?

(In reply to comment #2)
> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=94dae71c582130aa053c953311b604b6d8f04e28
> which required me to look both at the commit message and the full source to see
> what was going on. Would having a VALGRIND_TESTS variable make it clearer?

It would make this diff clearer, but at the cost of adding a somewhat
pointless layer of abstraction.

Explanation: we have two kinds of test in this directory: compiled C,
and Python. All the compiled tests are in $(noinst_PROGRAMS); TESTS
contains those plus the Python test. When valgrinding, we don't want to
valgrind the Python test.

Perhaps a clearer structure, which doesn't add a strange layer of
abstraction, would be:

dist_noinst_SCRIPTS = all-errors-documented.py
noinst_PROGRAMS = ...

TESTS = \
    $(noinst_PROGRAMS) \
    $(dist_noinst_SCRIPTS)

# skip the scripts when valgrinding - we don't want to valgrind the
# interpreter
check-valgrind:
	$(MAKE) check-TESTS \
		dist_noinst_SCRIPTS="" \
		TESTS_ENVIRONMENT="$(VALGRIND_TESTS_ENVIRONMENT)"
	$(MAKE) -C dbus check-valgrind

Would you be happier with that?

> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=7c0af0be354edad11930d2d0e6de897ae7a57253
> If we want to have a test that checks whether valgrind likes our program,
> wouldn't it be better to have it fail if valgrind doesn't like us?

You'd think... but in practice:

* any false positives in libc, libdbus, dbus-glib cause the tests to fail,
  and the rest of the tests not to run

* memory leaks don't count as an error, so they don't cause the tests to
  fail, so you have to grep the output anyway

> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=63ceaa9286a5b5ba1cf30b9954601140b6b7893d
> looks good if my guess at what the ... syntax means is correct.

"..." means "zero or more stack frames"; it's very useful, but I didn't know
about it when I wrote the initial suppressions file.

> -- Have you made any changes that could/should be published to
> http://live.gnome.org/Valgrind?

It should probably be referenced there and/or copied into Johan's
suppressions file, yes. I'd prefer to get it merged first, so we can
just point into gitweb...

Johan's suppressions are too strict too :-P

> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=8f69846e0eacf758f35eb3d8d1f6034e243e6bb2
> -- I like these kinds of commits.

Me too :-)

> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=2fa0d932088ca4d8812bbf3f46f787d51ff94730
> http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=2d11ca85e44f33f590b352559c3b8016d7673945
> -- Would it be possible to make our tests drop off the bus before exiting, or
> is this too much work?

Too much work for right now, certainly! MC jumps through hoops to get this
to work, but it took quite a bit of coding to get there.

You're not allowed to disconnect the shared session bus, so any test
that used tp_dbus_daemon_dup() or dbus_g_bus_get() or tp_get_bus() would
have these leaks anyway, unless we contrived some way to induce the session
bus to kick us off (which is liable to get us killed by libdbus assertion
failures).

> > http://git.collabora.co.uk/?p=user/smcv/telepathy-glib-smcv.git;a=commitdiff;h=94dae71c582130aa053c953311b604b6d8f04e28
> > which required me to look both at the commit message and the full source to see
> > what was going on. Would having a VALGRIND_TESTS variable make it clearer?
> It would make this diff clearer, but at the cost of adding a somewhat
> pointless layer of abstraction.

Ok. Leave it as it is then. I guess if it all fits on one page, then anyone who's ever used valgrind on a python interpreter will be able to glance at it and see what you're doing. 

Ship it.

I like this too.

Fixed in 0.9.2