From 5e97de0089e5a91e4dd8bae5aa7e1956597c508b Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Fri, 17 Jun 2016 17:50:49 -0400 Subject: [PATCH] Tests: spawn ht_cleanup thread in unit tests Run the cleanup thread in the unit tests to ensure that the hash tables are properly cleaned-up even in the context of tests. This allows us to reliably check for memory leaks in the unit tests. --- src/bin/lttng-sessiond/Makefile.am | 2 +- src/bin/lttng-sessiond/ht-cleanup.c | 136 +++++++++++++++++++++--- src/bin/lttng-sessiond/ht-cleanup.h | 26 +++++ src/bin/lttng-sessiond/lttng-sessiond.h | 12 +-- src/bin/lttng-sessiond/main.c | 89 ++-------------- tests/unit/Makefile.am | 3 + tests/unit/test_session.c | 9 ++ 7 files changed, 172 insertions(+), 105 deletions(-) create mode 100644 src/bin/lttng-sessiond/ht-cleanup.h diff --git a/src/bin/lttng-sessiond/Makefile.am b/src/bin/lttng-sessiond/Makefile.am index 7819222d9..29f48c9cc 100644 --- a/src/bin/lttng-sessiond/Makefile.am +++ b/src/bin/lttng-sessiond/Makefile.am @@ -25,7 +25,7 @@ lttng_sessiond_SOURCES = utils.c utils.h \ health-sessiond.h \ cmd.c cmd.h \ buffer-registry.c buffer-registry.h \ - testpoint.h ht-cleanup.c \ + testpoint.h ht-cleanup.c ht-cleanup.h \ snapshot.c snapshot.h \ agent.c agent.h \ save.h save.c \ diff --git a/src/bin/lttng-sessiond/ht-cleanup.c b/src/bin/lttng-sessiond/ht-cleanup.c index 4a4bfc41b..08bd27286 100644 --- a/src/bin/lttng-sessiond/ht-cleanup.c +++ b/src/bin/lttng-sessiond/ht-cleanup.c @@ -21,12 +21,77 @@ #include #include #include +#include #include "lttng-sessiond.h" #include "health-sessiond.h" #include "testpoint.h" +#include "utils.h" -void *thread_ht_cleanup(void *data) +int ht_cleanup_quit_pipe[2] = { -1, -1 }; + +/* + * Check if the ht_cleanup thread quit pipe was triggered. + * + * Return true if it was triggered else false; + */ +static bool check_quit_pipe(int fd, uint32_t events) +{ + return (fd == ht_cleanup_quit_pipe[0] && (events & LPOLLIN)); +} + +static int init_pipe(int *pipe_fds) +{ + int ret, i; + + ret = pipe(pipe_fds); + if (ret < 0) { + PERROR("ht_cleanup thread quit pipe"); + goto error; + } + + for (i = 0; i < 2; i++) { + ret = fcntl(pipe_fds[i], F_SETFD, FD_CLOEXEC); + if (ret < 0) { + PERROR("fcntl ht_cleanup_quit_pipe"); + goto error; + } + } +error: + return ret; +} + +/* + * Create a poll set with O_CLOEXEC and add the thread quit pipe to the set. + */ +static int set_pollset(struct lttng_poll_event *events, size_t size) +{ + int ret; + + ret = lttng_poll_create(events, size, LTTNG_CLOEXEC); + if (ret < 0) { + goto error; + } + + ret = lttng_poll_add(events, ht_cleanup_quit_pipe[0], + LPOLLIN | LPOLLERR); + if (ret < 0) { + goto error; + } + + ret = lttng_poll_add(events, ht_cleanup_pipe[0], LPOLLIN | LPOLLERR); + if (ret < 0) { + DBG("[ht-thread] lttng_poll_add error %d.", ret); + goto error; + } + + return 0; + +error: + return ret; +} + +static void *thread_ht_cleanup(void *data) { int ret, i, pollfd, err = -1; ssize_t size_ret; @@ -47,26 +112,16 @@ void *thread_ht_cleanup(void *data) health_code_update(); - ret = sessiond_set_ht_cleanup_thread_pollset(&events, 2); + ret = set_pollset(&events, 2); if (ret < 0) { DBG("[ht-thread] sessiond_set_ht_cleanup_thread_pollset error %d.", ret); goto error_poll_create; } - /* Add pipe to the pollset. */ - ret = lttng_poll_add(&events, ht_cleanup_pipe[0], LPOLLIN | LPOLLERR); - if (ret < 0) { - DBG("[ht-thread] lttng_poll_add error %d.", ret); - goto error; - } - health_code_update(); while (1) { DBG3("[ht-thread] Polling."); - - /* Inifinite blocking call, waiting for transmission */ -restart: health_poll_entry(); ret = lttng_poll_wait(&events, -1); DBG3("[ht-thread] Returning from poll on %d fds.", @@ -77,13 +132,12 @@ restart: * Restart interrupted system call. */ if (errno == EINTR) { - goto restart; + continue; } goto error; } nb_fd = ret; - for (i = 0; i < nb_fd; i++) { struct lttng_ht *ht; @@ -146,7 +200,7 @@ restart: } /* Thread quit pipe has been closed. Killing thread. */ - ret = sessiond_check_ht_cleanup_quit(pollfd, revents); + ret = check_quit_pipe(pollfd, revents); if (ret) { err = 0; DBG("[ht-cleanup] quit."); @@ -170,3 +224,55 @@ error_testpoint: rcu_unregister_thread(); return NULL; } + +int init_ht_cleanup_thread(pthread_t *thread) +{ + int ret; + + ret = init_pipe(ht_cleanup_pipe); + if (ret) { + goto error; + } + + init_pipe(ht_cleanup_quit_pipe); + if (ret) { + goto error_quit_pipe; + } + + ret = pthread_create(thread, NULL, thread_ht_cleanup, NULL); + if (ret) { + errno = ret; + PERROR("pthread_create ht_cleanup"); + goto error_thread; + } + +error: + return ret; + +error_thread: + utils_close_pipe(ht_cleanup_quit_pipe); +error_quit_pipe: + utils_close_pipe(ht_cleanup_pipe); + return ret; +} + +int fini_ht_cleanup_thread(pthread_t *thread) +{ + int ret; + + ret = notify_thread_pipe(ht_cleanup_quit_pipe[1]); + if (ret < 0) { + ERR("write error on ht_cleanup quit pipe"); + goto end; + } + + ret = pthread_join(*thread, NULL); + if (ret) { + errno = ret; + PERROR("pthread_join ht cleanup thread"); + } + utils_close_pipe(ht_cleanup_pipe); + utils_close_pipe(ht_cleanup_quit_pipe); +end: + return ret; +} diff --git a/src/bin/lttng-sessiond/ht-cleanup.h b/src/bin/lttng-sessiond/ht-cleanup.h new file mode 100644 index 000000000..8ca0e3123 --- /dev/null +++ b/src/bin/lttng-sessiond/ht-cleanup.h @@ -0,0 +1,26 @@ +/* + * Copyright (C) 2016 - Jérémie Galarneau + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License, version 2 only, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#ifndef _LTTNG_HT_CLEANUP_H +#define _LTTNG_HT_CLEANUP_H + +#include + +int init_ht_cleanup_thread(pthread_t *thread); +int fini_ht_cleanup_thread(pthread_t *thread); + +#endif /* _LTTNG_HT_CLEANUP_H */ diff --git a/src/bin/lttng-sessiond/lttng-sessiond.h b/src/bin/lttng-sessiond/lttng-sessiond.h index 07a6edde0..65ab37d59 100644 --- a/src/bin/lttng-sessiond/lttng-sessiond.h +++ b/src/bin/lttng-sessiond/lttng-sessiond.h @@ -110,6 +110,9 @@ extern long page_size; */ extern unsigned int agent_tcp_port; +/* Application health monitoring */ +extern struct health_app *health_sessiond; + /* * Section name to look for in the daemon configuration file. */ @@ -118,15 +121,8 @@ extern const char * const config_section_name; /* Is this daemon root or not. */ extern int is_root; -int sessiond_set_thread_pollset(struct lttng_poll_event *events, size_t size); int sessiond_check_thread_quit_pipe(int fd, uint32_t events); - -int sessiond_set_ht_cleanup_thread_pollset(struct lttng_poll_event *events, - size_t size); -int sessiond_check_ht_cleanup_quit(int fd, uint32_t events); - -void *thread_ht_cleanup(void *data); - +int sessiond_set_thread_pollset(struct lttng_poll_event *events, size_t size); void sessiond_notify_ready(void); #endif /* _LTT_SESSIOND_H */ diff --git a/src/bin/lttng-sessiond/main.c b/src/bin/lttng-sessiond/main.c index 0c97b1cb6..497f4ee97 100644 --- a/src/bin/lttng-sessiond/main.c +++ b/src/bin/lttng-sessiond/main.c @@ -72,6 +72,7 @@ #include "load-session-thread.h" #include "syscall.h" #include "agent.h" +#include "ht-cleanup.h" #define CONSUMERD_FILE "lttng-consumerd" @@ -189,7 +190,6 @@ static int kernel_poll_pipe[2] = { -1, -1 }; * for all threads when receiving an event on the pipe. */ static int thread_quit_pipe[2] = { -1, -1 }; -static int ht_cleanup_quit_pipe[2] = { -1, -1 }; /* * This pipe is used to inform the thread managing application communication @@ -315,6 +315,11 @@ struct lttng_ht *agent_apps_ht_by_sock = NULL; #define NR_LTTNG_SESSIOND_READY 3 int lttng_sessiond_ready = NR_LTTNG_SESSIOND_READY; +int sessiond_check_thread_quit_pipe(int fd, uint32_t events) +{ + return (fd == thread_quit_pipe[0] && (events & LPOLLIN)) ? 1 : 0; +} + /* Notify parents that we are ready for cmd and health check */ LTTNG_HIDDEN void sessiond_notify_ready(void) @@ -421,47 +426,6 @@ int sessiond_set_thread_pollset(struct lttng_poll_event *events, size_t size) return __sessiond_set_thread_pollset(events, size, thread_quit_pipe); } -/* - * Create a poll set with O_CLOEXEC and add the thread quit pipe to the set. - */ -int sessiond_set_ht_cleanup_thread_pollset(struct lttng_poll_event *events, - size_t size) -{ - return __sessiond_set_thread_pollset(events, size, - ht_cleanup_quit_pipe); -} - -static -int __sessiond_check_thread_quit_pipe(int fd, uint32_t events, int a_pipe) -{ - if (fd == a_pipe && (events & LPOLLIN)) { - return 1; - } - return 0; -} - -/* - * Check if the thread quit pipe was triggered. - * - * Return 1 if it was triggered else 0; - */ -int sessiond_check_thread_quit_pipe(int fd, uint32_t events) -{ - return __sessiond_check_thread_quit_pipe(fd, events, - thread_quit_pipe[0]); -} - -/* - * Check if the ht_cleanup thread quit pipe was triggered. - * - * Return 1 if it was triggered else 0; - */ -int sessiond_check_ht_cleanup_quit(int fd, uint32_t events) -{ - return __sessiond_check_thread_quit_pipe(fd, events, - ht_cleanup_quit_pipe[0]); -} - /* * Init thread quit pipe. * @@ -494,11 +458,6 @@ static int init_thread_quit_pipe(void) return __init_thread_quit_pipe(thread_quit_pipe); } -static int init_ht_cleanup_quit_pipe(void) -{ - return __init_thread_quit_pipe(ht_cleanup_quit_pipe); -} - /* * Stop all threads by closing the thread quit pipe. */ @@ -5651,23 +5610,8 @@ int main(int argc, char **argv) goto exit_health_sessiond_cleanup; } - if (init_ht_cleanup_quit_pipe()) { - retval = -1; - goto exit_ht_cleanup_quit_pipe; - } - - /* Setup the thread ht_cleanup communication pipe. */ - if (utils_create_pipe_cloexec(ht_cleanup_pipe)) { - retval = -1; - goto exit_ht_cleanup_pipe; - } - /* Create thread to clean up RCU hash tables */ - ret = pthread_create(&ht_cleanup_thread, NULL, - thread_ht_cleanup, (void *) NULL); - if (ret) { - errno = ret; - PERROR("pthread_create ht_cleanup"); + if (init_ht_cleanup_thread(&ht_cleanup_thread)) { retval = -1; goto exit_ht_cleanup; } @@ -6227,29 +6171,12 @@ exit_init_data: */ rcu_barrier(); - ret = notify_thread_pipe(ht_cleanup_quit_pipe[1]); - if (ret < 0) { - ERR("write error on ht_cleanup quit pipe"); - retval = -1; - } - - ret = pthread_join(ht_cleanup_thread, &status); + ret = fini_ht_cleanup_thread(&ht_cleanup_thread); if (ret) { - errno = ret; - PERROR("pthread_join ht cleanup thread"); retval = -1; } exit_ht_cleanup: - utils_close_pipe(ht_cleanup_pipe); -exit_ht_cleanup_pipe: - - /* - * Close the ht_cleanup quit pipe. - */ - utils_close_pipe(ht_cleanup_quit_pipe); -exit_ht_cleanup_quit_pipe: - health_app_destroy(health_sessiond); exit_health_sessiond_cleanup: exit_create_run_as_worker_cleanup: diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am index 3355e59b3..f9dd53bef 100644 --- a/tests/unit/Makefile.am +++ b/tests/unit/Makefile.am @@ -46,7 +46,10 @@ SESSIONS=$(top_builddir)/src/bin/lttng-sessiond/session.$(OBJEXT) \ $(top_builddir)/src/bin/lttng-sessiond/consumer.$(OBJEXT) \ $(top_builddir)/src/bin/lttng-sessiond/utils.$(OBJEXT) \ $(top_builddir)/src/bin/lttng-sessiond/snapshot.$(OBJEXT) \ + $(top_builddir)/src/bin/lttng-sessiond/ht-cleanup.$(OBJEXT) \ $(top_builddir)/src/common/libcommon.la \ + $(top_builddir)/src/common/testpoint/libtestpoint.la \ + $(top_builddir)/src/common/compat/libcompat.la \ $(top_builddir)/src/common/health/libhealth.la \ $(top_builddir)/src/common/sessiond-comm/libsessiond-comm.la diff --git a/tests/unit/test_session.c b/tests/unit/test_session.c index 7c925908d..f2343c991 100644 --- a/tests/unit/test_session.c +++ b/tests/unit/test_session.c @@ -29,6 +29,8 @@ #include #include +#include +#include #include #include @@ -40,7 +42,9 @@ /* Number of TAP tests in this file */ #define NUM_TESTS 11 +struct health_app *health_sessiond; static struct ltt_session_list *session_list; +static pthread_t ht_cleanup_thread; /* For error.h */ int lttng_opt_quiet = 1; @@ -293,6 +297,9 @@ int main(int argc, char **argv) { plan_tests(NUM_TESTS); + health_sessiond = health_app_create(NR_HEALTH_SESSIOND_TYPES); + assert(!init_ht_cleanup_thread(&ht_cleanup_thread)); + diag("Sessions unit tests"); test_session_list(); @@ -311,5 +318,7 @@ int main(int argc, char **argv) test_large_session_number(); + assert(!fini_ht_cleanup_thread(&ht_cleanup_thread)); + return exit_status(); } -- 2.34.1