Summary of the referenced branch (branched after Jonny's last commit): * Fix a leaked reference to the default TpDBusDaemon in tp_account_manager_dup * Fix some leaks in the example "callable" CM * Fix some leaks in the tests * Annotate some deliberate one-per-process "leaks" * Move the suppressions file to tools/ and improve it * Run valgrind under a better environment, and with better options
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
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.