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

[Eoff, Ullysses A]

>-----Original Message-----
>From: Kristian Hoegsberg [mailto:hoegsberg at gmail.com]
>Sent: Thursday, March 22, 2012 7:14 PM
>To: Eoff, Ullysses A
>Cc: wayland-devel at lists.freedesktop.org
>Subject: Re: [PATCH] Add simple memory leak check to all tests.
>
>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
>

Kristian,

Thanks for the feedback.  Yes, coding conventions are good and
I'm more than willing to follow whatever style is used for the project.
My usual coding style tends to follow the C++ "Red Book" and habit just
kicked in.  I'll resubmit a new patch with your suggested style fixes.

Further comments are inline.

Thanks,
U. Artie
 

>> 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.
>

Habit again :)... I'll create a new profile in my editor to use 8 wide hard tabs.

>>  }
>> 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.
>

Yes, you are right.  We don't need these prototypes.  I'll remove them.

>>  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