<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On 1 December 2014 at 11:42, Pekka Paalanen <span dir="ltr"><<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Fri, 21 Nov 2014 11:18:33 +0100<br>
Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>> wrote:<br>
<br>
> We didn't free the struct client that we got from client_connect()<br>
><br>
> Signed-off-by: Marek Chalupa <<a href="mailto:mchqwerty@gmail.com">mchqwerty@gmail.com</a>><br>
> ---<br>
> tests/display-test.c | 23 +++++++++++++----------<br>
> tests/test-compositor.c | 1 +<br>
> 2 files changed, 14 insertions(+), 10 deletions(-)<br>
<br>
</span>Hi,<br>
<br>
how did you find these leaks?<br></blockquote><div><br></div><div>By valgrind --trace-children=yes<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
More importantly, why doesn't our leak checker find these?<br>
That is, the code in test-runner.c, run_test().<br></blockquote><div><br></div><div>Because it is in another fork. It looks like this:<br><br></div><div>run_test()<br></div><div> int the alloc_count and fds_count<br> run the TEST function<br></div><div> create_client --> fork<br></div><div> [in fork] alloc, dealloc, whatever<br></div><div> wait for client<br></div><div> check alloc_count and fds_count<br><br></div><div>The problem is that the forked client has the alloc_count and fds_count<br></div><div>in its own virtual memory, so the leak checker finds only leaks in<br></div><div>TEST function.<br><br>I have some patches that extend these leak checks<br></div><div>into the client too, but I ran into problem with filedescriptors there.<br></div><div>The problem is that when forking the client, we pass WAYLAND_SOCKET fd<br>to wl_display_connect(). wl_display_connect() just uses this fd (no dup),<br>so wl_display_disconnect() closes this fd.<br>So the result of leak checker is that the client closed<br></div><div>one more fd that it opened. Hardcoding -1 to client's fds number<br></div><div>is not a solution too, because client does not have to call wl_display_connect()<br><br></div><div>So summing it up: using leak checker in clients is possible, but only for memory<br></div><div>leaks. For fd leaks we'd have to use some more sophisticated solution.<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
For one, I think those are not thread-safe. Calling exit() in a test<br>
would bypass it too, but I'm not sure that happens.<br></blockquote><div> </div><div>Huh? What is not thread-safe? you mean leak checks when running only one test (without fork?)<br></div><div>Yes, it looks like when calling exit in the test, the leak check is bypassed.<br></div><div>Quick grep though the tests shows that there are no calls to exit in TEST, just in<br></div><div>the forked clients (which is not a problem _now_ with no leak checks in clients)<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The Valgrind output nowadays is quite messy. Even when I run a single<br>
test from one test binary which should not fork at all, I think I see<br>
two forks in Valgrind output: one is likely the debugger check, maybe<br>
the other is actually a thread for the test compositor or something?<br></blockquote><div><br></div><div>Yes, the first fork is the debugger check and the other is the client<br></div><div>for test compositor. In the tests without the test compositor you should see only<br></div><div>one fork (the debugger check)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I think Valgrind is saying that there are many more leaks to plug.<br>
<br></blockquote><div> </div><div>Yes<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
In any case, this patch pushed.<br>
<br>
Thanks,<br>
pq<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> diff --git a/tests/display-test.c b/tests/display-test.c<br>
> index aecf341..776a9c9 100644<br>
> --- a/tests/display-test.c<br>
> +++ b/tests/display-test.c<br>
> @@ -155,6 +155,14 @@ bind_seat(struct wl_client *client, void *data,<br>
> }<br>
><br>
> static void<br>
> +client_disconnect_nocheck(struct client *c)<br>
> +{<br>
> + wl_proxy_destroy((struct wl_proxy *) c->tc);<br>
> + wl_display_disconnect(c->wl_display);<br>
> + free(c);<br>
> +}<br>
> +<br>
> +static void<br>
> post_error_main(void)<br>
> {<br>
> struct client *c = client_connect();<br>
> @@ -170,8 +178,7 @@ post_error_main(void)<br>
> /* don't call client_disconnect(c), because then the test would be<br>
> * aborted due to checks for error in this function */<br>
> wl_proxy_destroy((struct wl_proxy *) seat);<br>
> - wl_proxy_destroy((struct wl_proxy *) c->tc);<br>
> - wl_display_disconnect(c->wl_display);<br>
> + client_disconnect_nocheck(c);<br>
> }<br>
><br>
> TEST(post_error_to_one_client)<br>
> @@ -224,8 +231,7 @@ post_error_main3(void)<br>
> /* don't call client_disconnect(c), because then the test would be<br>
> * aborted due to checks for error in this function */<br>
> wl_proxy_destroy((struct wl_proxy *) seat);<br>
> - wl_proxy_destroy((struct wl_proxy *) c->tc);<br>
> - wl_display_disconnect(c->wl_display);<br>
> + client_disconnect_nocheck(c);<br>
> }<br>
><br>
> /* all the testcases could be in one TEST, but splitting it<br>
> @@ -294,8 +300,7 @@ post_nomem_main(void)<br>
> assert(wl_display_get_error(c->wl_display) == ENOMEM);<br>
><br>
> wl_proxy_destroy((struct wl_proxy *) seat);<br>
> - wl_proxy_destroy((struct wl_proxy *) c->tc);<br>
> - wl_display_disconnect(c->wl_display);<br>
> + client_disconnect_nocheck(c);<br>
> }<br>
><br>
> TEST(post_nomem_tst)<br>
> @@ -413,8 +418,7 @@ threading_post_err(void)<br>
> test_set_timeout(3);<br>
> pthread_join(thread, NULL);<br>
><br>
> - wl_proxy_destroy((struct wl_proxy *) c->tc);<br>
> - wl_display_disconnect(c->wl_display);<br>
> + client_disconnect_nocheck(c);<br>
> }<br>
><br>
> TEST(threading_errors_tst)<br>
> @@ -565,8 +569,7 @@ threading_read_after_error(void)<br>
> test_set_timeout(3);<br>
> pthread_join(thread, NULL);<br>
><br>
> - wl_proxy_destroy((struct wl_proxy *) c->tc);<br>
> - wl_display_disconnect(c->wl_display);<br>
> + client_disconnect_nocheck(c);<br>
> }<br>
><br>
> TEST(threading_read_after_error_tst)<br>
> diff --git a/tests/test-compositor.c b/tests/test-compositor.c<br>
> index 3248e2d..6f86a85 100644<br>
> --- a/tests/test-compositor.c<br>
> +++ b/tests/test-compositor.c<br>
> @@ -452,6 +452,7 @@ client_disconnect(struct client *c)<br>
><br>
> wl_proxy_destroy((struct wl_proxy *) c->tc);<br>
> wl_display_disconnect(c->wl_display);<br>
> + free(c);<br>
> }<br>
><br>
> /* num is number of clients that requests to stop display.<br>
<br></div></div></blockquote><div> </div><div>Thanks,<br>Marek <br></div></div><br></div></div>