[PATCH] Add simple memory leak check to all tests.

[Kristian Hoegsberg]

On Thu, Mar 22, 2012 at 01:37:20PM -0700, U. Artie Eoff wrote:
> From: "U. Artie Eoff" <ullysses.a.eoff at intel.com>
> 
> Wrap all test cases with a memory balance check to
> detect potential memory leaks.
> Fixed a few tests that had memory leaks contained
> in the test themselves.

Thanks Artie, that's cool and good to see that the check works and
already caught a few leaks.

There's a couple of stylistic issues with the patch, and in short it
comes down to "please follow the existing coding convention".  We
don't have a coding convention document, but the coding style is very
consistent, so if in doubt, look at the existing code for an example.
Specific comments below.

thanks,
Kristian

> Signed-off-by: U. Artie Eoff <ullysses.a.eoff at intel.com>
> ---
>  tests/Makefile.am   |    2 +-
>  tests/array-test.c  |    1 +
>  tests/map-test.c    |    2 ++
>  tests/test-runner.c |   34 ++++++++++++++++++++++++++++++++++
>  tests/test-runner.h |    8 ++++++++
>  5 files changed, 46 insertions(+), 1 deletions(-)
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index cf81c0e..d7d12e0 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -12,5 +12,5 @@ event_loop_test_SOURCES = event-loop-test.c test-runner.c
>  AM_CFLAGS = $(GCC_CFLAGS)
>  LDADD = $(top_builddir)/src/libwayland-util.la \
>  	$(top_builddir)/src/libwayland-server.la \
> -	-lrt $(FFI_LIBS)
> +	-lrt -ldl $(FFI_LIBS)
>  
> diff --git a/tests/array-test.c b/tests/array-test.c
> index cb62713..16d73b7 100644
> --- a/tests/array-test.c
> +++ b/tests/array-test.c
> @@ -133,4 +133,5 @@ TEST(array_for_each)
>  	wl_array_for_each(p, &array)
>  		assert(*p == elements[i++]);
>  	assert(i == 5);
> +    wl_array_release(&array);

We more or less follow the kernel indent style, that is tabs are 8
wide and we use the actual tab character, not 8 spaces.  I don't know
if you use an editor that show tabs as 4 spaces, but in emacs/vi/gedit
the above wl_array_release() does not align with the rest of the code.

>  }
> diff --git a/tests/map-test.c b/tests/map-test.c
> index 5ef940f..c171fbd 100644
> --- a/tests/map-test.c
> +++ b/tests/map-test.c
> @@ -90,4 +90,6 @@ TEST(map_remove)
>  	l = wl_map_insert_new(&map, WL_MAP_SERVER_SIDE, &d);
>  	assert(l == WL_SERVER_ID_START + 1);
>  	assert(wl_map_lookup(&map, l) == &d);
> +
> +    wl_map_release(&map);
>  }
> diff --git a/tests/test-runner.c b/tests/test-runner.c
> index 301c736..5adb697 100644
> --- a/tests/test-runner.c
> +++ b/tests/test-runner.c
> @@ -20,6 +20,8 @@
>   * OF THIS SOFTWARE.
>   */
>  
> +#define _GNU_SOURCE
> +
>  #include <unistd.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -27,10 +29,40 @@
>  #include <sys/wait.h>
>  #include <string.h>
>  #include <assert.h>
> +#include <dlfcn.h>
> +
>  #include "test-runner.h"
>  
> +static int num_alloc                = 0;
> +static void* (*sys_malloc)(size_t)  = NULL;
> +static void (*sys_free)(void*)      = NULL;

No need to initialize static variables to 0/NULL;

>  extern const struct test __start_test_section, __stop_test_section;
>  
> +void* malloc(size_t size)
> +{
> +    if (NULL == sys_malloc)

We don't use yoda conditions; they make the code unintuitive and
harder to read and gcc will warn against accidental assignments.

> +    {

Opening braces are on the same line as the if statement, and in case
of an if-body with just one statement we generally avoid them.

> +        sys_malloc = (void* (*)(size_t))(dlsym(RTLD_NEXT, "malloc"));
> +    }
> +    num_alloc++;
> +    return sys_malloc(size);
> +}
> +
> +void free(void* mem)
> +{
> +    if (NULL == sys_free)
> +    {
> +        sys_free =  (void (*)(void*))(dlsym(RTLD_NEXT, "free"));
> +    }
> +    if (NULL != mem)
> +    {
> +        num_alloc--;
> +    }
> +
> +    sys_free(mem);
> +}
> +
>  static const struct test *
>  find_test(const char *name)
>  {
> @@ -46,7 +78,9 @@ find_test(const char *name)
>  static void
>  run_test(const struct test *t)
>  {
> +    int cur_alloc = num_alloc;

We always have an empty line between variable declarations and the
code.

>  	t->run();
> +    assert(("memory leak detected in test.", cur_alloc == num_alloc));
>  	exit(EXIT_SUCCESS);
>  }
>  
> diff --git a/tests/test-runner.h b/tests/test-runner.h
> index c1fdff9..b859026 100644
> --- a/tests/test-runner.h
> +++ b/tests/test-runner.h
> @@ -1,3 +1,9 @@
> +#ifndef _TEST_RUNNER_H_
> +#define _TEST_RUNNER_H_
> +
> +inline void* malloc(size_t size);
> +inline void free(void* mem);
> +

This looks wrong, we don't need these prototypes.

>  struct test {
>  	const char *name;
>  	void (*run)(void);
> @@ -12,3 +18,5 @@ struct test {
>  	};							\
>  								\
>  	static void name(void)
> +
> +#endif
> -- 
> 1.7.7.6
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel