[Libguestfs] [PATCH nbdkit 2/4] valgrind: Add --show-leak-kinds=all and comprehensive list of suppressions.

[Eric Blake]

On 12/2/18 10:39 AM, Richard W.M. Jones wrote:
> By default valgrind suppresses many leaks.  I'm not even sure exactly
> how it decides which ones to suppress, but certainly global variables
> pointing to malloc’d data are suppressed, which is not useful
> behaviour.

Here's my understanding of why that is the default - if you assign 
malloc()d storage into a global, then that storage is in scope right up 
until you call exit().  And it's faster (and often easier) to exit() a 
process with memory still allocated (the OS will clean it up anyways) 
than it is to manually chase down the memory cleanups yourself.

> 
> Tell valgrind to show all leaks.  It won't give an error on them.
> 
> However to do this we also need a much more comprehensive list of
> suppressions so that we don't constantly show unavoidable leaks in
> (eg) dlopen.  This adds separate files for each class of suppressions
> in a new valgrind/ directory.
> ---
>   .gitignore                                    |   1 +
>   Makefile.am                                   |   2 +-
>   configure.ac                                  |   3 +-
>   valgrind/Makefile.am                          |  47 ++++++++
>   valgrind/glibc.suppressions                   | 106 ++++++++++++++++++
>   .../nbdkit.suppressions                       |  21 +---
>   valgrind/perl.suppressions                    |  40 +++++++
>   wrapper.c                                     |   3 +-
>   8 files changed, 205 insertions(+), 18 deletions(-)
> 

> +++ b/valgrind/Makefile.am
> @@ -0,0 +1,47 @@

> +include $(top_srcdir)/common-rules.mk
> +
> +suppressions_files = $(wildcard *.suppressions)

A GNU make-ism - but you already mention requiring GNU make in README. 
Should we make ./configure error out hard if $MAKE is not GNU Make, 
rather than risking someone getting 80% though a build on BSD make and 
then choking when it gets here?

> +++ b/wrapper.c
> @@ -130,8 +130,9 @@ main (int argc, char *argv[])
>       passthru (VALGRIND);
>       passthru ("--vgdb=no");
>       passthru ("--leak-check=full");
> +    passthru ("--show-leak-kinds=all");

I could understand this if we were implementing a library and wanted to 
make it easier for some other user to call our library shutdown to 
reclaim all memory that we otherwise stashed in globals - but when we 
are just a standalone app, do we really need to worry about memory still 
reachable in globals, as that's not a true leak?

>       passthru ("--error-exitcode=119");
> -    passthru_format ("--suppressions=%s/valgrind-suppressions", srcdir);
> +    passthru_format ("--suppressions=%s/valgrind/suppressions", builddir);

Is this still right under VPATH?

>       passthru ("--trace-children=no");
>       passthru ("--run-libc-freeres=no");
>       passthru ("--num-callers=20");
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org