From 0111df3767df67ee2bad3647abb985028400429f Mon Sep 17 00:00:00 2001 From: Markus Makela Date: Sat, 23 Jan 2016 02:56:04 +0200 Subject: [PATCH] Cleaned up the use of thread types The THREAD type was not used everywhere and pthread_t was used instead. The thread creation function also returned the address of a stack allocated value which isn't guaranteed to be usable. --- server/core/gateway.c | 27 +++++++++++++++++++------- server/core/housekeeper.c | 7 ++++++- server/core/spinlock.c | 4 ++-- server/core/test/testspinlock.c | 8 ++++---- server/core/thread.c | 21 +++++++++----------- server/include/thread.h | 15 ++++++++------ server/modules/monitor/galeramon.c | 8 ++++++-- server/modules/monitor/galeramon.h | 2 +- server/modules/monitor/mmmon.c | 9 +++++++-- server/modules/monitor/mmmon.h | 2 +- server/modules/monitor/mysql_mon.c | 8 ++++++-- server/modules/monitor/mysqlmon.h | 2 +- server/modules/monitor/ndbclustermon.c | 9 +++++++-- 13 files changed, 79 insertions(+), 43 deletions(-) diff --git a/server/core/gateway.c b/server/core/gateway.c index 41fb828ce..4381b8858 100644 --- a/server/core/gateway.c +++ b/server/core/gateway.c @@ -1059,13 +1059,13 @@ int main(int argc, char **argv) int daemon_pipe[2] = {-1, -1}; bool parent_process; int child_status; - void** threads = NULL; /*< thread list */ + THREAD* threads = NULL; /*< thread list */ char mysql_home[PATH_MAX+1]; char datadir_arg[10+PATH_MAX+1]; /*< '--datadir=' + PATH_MAX */ char language_arg[11+PATH_MAX+1]; /*< '--language=' + PATH_MAX */ char* cnf_file_path = NULL; /*< conf file, to be freed */ char* cnf_file_arg = NULL; /*< conf filename from cmd-line arg */ - void* log_flush_thr = NULL; + THREAD log_flush_thr; char* tmp_path; char* tmp_var; int option_index; @@ -1940,9 +1940,14 @@ int main(int argc, char **argv) * Start periodic log flusher thread. */ log_flush_timeout_ms = 1000; - log_flush_thr = thread_start( - log_flush_cb, - (void *)&log_flush_timeout_ms); + + if (thread_start(&log_flush_thr, log_flush_cb, (void *) &log_flush_timeout_ms) == NULL) + { + char* logerr = "Failed to start log flushing thread."; + print_log_n_stderr(true, !daemon_mode, logerr, logerr, 0); + rc = MAXSCALE_INTERNALERROR; + goto return_main; + } /* * Start the housekeeper thread @@ -1954,14 +1959,22 @@ int main(int argc, char **argv) * configured as the main thread will also poll. */ n_threads = config_threadcount(); - threads = (void **)calloc(n_threads, sizeof(void *)); + threads = calloc(n_threads, sizeof(THREAD)); /*< * Start server threads. */ for (thread_id = 0; thread_id < n_threads - 1; thread_id++) { - threads[thread_id] = thread_start(worker_thread_main, (void *)(thread_id + 1)); + if (thread_start(&threads[thread_id], worker_thread_main, + (void *)(thread_id + 1)) == NULL) + { + char* logerr = "Failed to start worker thread."; + print_log_n_stderr(true, !daemon_mode, logerr, logerr, 0); + rc = MAXSCALE_INTERNALERROR; + goto return_main; + } } + MXS_NOTICE("MaxScale started with %d server threads.", config_threadcount()); /** * Successful start, notify the parent process that it can exit. diff --git a/server/core/housekeeper.c b/server/core/housekeeper.c index 98c4f2344..e3c4603c5 100644 --- a/server/core/housekeeper.c +++ b/server/core/housekeeper.c @@ -20,6 +20,7 @@ #include #include #include +#include /** * @file housekeeper.c Provide a mechanism to run periodic tasks @@ -54,6 +55,7 @@ static SPINLOCK tasklock = SPINLOCK_INIT; static int do_shutdown = 0; long hkheartbeat = 0; /*< One heartbeat is 100 milliseconds */ +static THREAD hk_thr_handle; static void hkthread(void *); @@ -63,7 +65,10 @@ static void hkthread(void *); void hkinit() { - thread_start(hkthread, NULL); + if (thread_start(&hk_thr_handle, hkthread, NULL) == NULL) + { + MXS_ERROR("Failed to start housekeeper thread."); + } } /** diff --git a/server/core/spinlock.c b/server/core/spinlock.c index adfcdc7ae..b8cd2fba7 100644 --- a/server/core/spinlock.c +++ b/server/core/spinlock.c @@ -87,7 +87,7 @@ spinlock_acquire(SPINLOCK *lock) } } lock->acquired++; - lock->owner = THREAD_SHELF(); + lock->owner = thread_self(); atomic_add(&(lock->waiting), -1); #endif } @@ -112,7 +112,7 @@ spinlock_acquire_nowait(SPINLOCK *lock) #endif #if SPINLOCK_PROFILE lock->acquired++; - lock->owner = THREAD_SHELF(); + lock->owner = thread_self(); #endif return TRUE; } diff --git a/server/core/test/testspinlock.c b/server/core/test/testspinlock.c index 3f2357d4a..71538f23d 100644 --- a/server/core/test/testspinlock.c +++ b/server/core/test/testspinlock.c @@ -111,7 +111,7 @@ static int test2() { SPINLOCK lck; -void *handle; +THREAD handle; struct timespec sleeptime; sleeptime.tv_sec = 10; @@ -120,7 +120,7 @@ struct timespec sleeptime; acquire_time = 0; spinlock_init(&lck); spinlock_acquire(&lck); - handle = thread_start(test2_helper, (void *)&lck); + thread_start(&handle, test2_helper, (void *)&lck); nanosleep(&sleeptime, NULL); spinlock_release(&lck); thread_wait(handle); @@ -206,7 +206,7 @@ static int test3() { // SPINLOCK lck; -void *handle[THREADS]; +THREAD handle[THREADS]; int i; int tnum[THREADS]; time_t rawtime; @@ -220,7 +220,7 @@ time_t rawtime; for (i = 0; i -#include + /** * @file thread.c - Implementation of thread related operations * @@ -33,20 +33,18 @@ /** * Start a polling thread * - * @param entry The entry point to call - * @param arg The argument to pass the thread entry point - * @return The thread handle + * @param thd Pointer to the THREAD object + * @param entry The entry point to call + * @param arg The argument to pass the thread entry point + * @return The thread handle or NULL if an error occurred */ -void * -thread_start(void (*entry)(void *), void *arg) +THREAD *thread_start(THREAD *thd, void (*entry)(void *), void *arg) { - pthread_t thd; - - if (pthread_create(&thd, NULL, (void *(*)(void *))entry, arg) != 0) + if (pthread_create(thd, NULL, (void *(*)(void *))entry, arg) != 0) { return NULL; } - return (void *)thd; + return thd; } /** @@ -55,7 +53,7 @@ thread_start(void (*entry)(void *), void *arg) * @param thd The thread handle */ void -thread_wait(void *thd) +thread_wait(THREAD thd) { void *rval; @@ -71,7 +69,6 @@ void thread_millisleep(int ms) { struct timespec req; - req.tv_sec = ms / 1000; req.tv_nsec = (ms % 1000) * 1000000; nanosleep(&req, NULL); diff --git a/server/include/thread.h b/server/include/thread.h index b8ba8471a..b982263d6 100644 --- a/server/include/thread.h +++ b/server/include/thread.h @@ -17,22 +17,25 @@ * * Copyright MariaDB Corporation Ab 2013-2014 */ -#include /** * @file thread.h The gateway threading interface * * An encapsulation of the threading used by the gateway. This is designed to - * isolate the majority of the gateway code from th epthread library, enabling + * isolate the majority of the gateway code from the pthread library, enabling * the gateway to be ported to a different threading package with the minimum * of changes. */ -#define THREAD pthread_t -#define THREAD_SHELF pthread_self +/** + * Thread type and thread identifier function macros + */ +#include +#define THREAD pthread_t +#define thread_self() pthread_self() -extern void *thread_start(void (*entry)(void *), void *arg); -extern void thread_wait(void *thd); +extern THREAD *thread_start(THREAD *thd, void (*entry)(void *), void *arg); +extern void thread_wait(THREAD thd); extern void thread_millisleep(int ms); #endif diff --git a/server/modules/monitor/galeramon.c b/server/modules/monitor/galeramon.c index 24596bc6b..6bbf5bab7 100644 --- a/server/modules/monitor/galeramon.c +++ b/server/modules/monitor/galeramon.c @@ -200,7 +200,11 @@ startMonitor(void *arg, void* opt) memset(handle->events, true, sizeof(handle->events)); } - handle->tid = (THREAD) thread_start(monitorMain, mon); + if (thread_start(&handle->thread, monitorMain, mon) == NULL) + { + MXS_ERROR("Failed to start monitor thread for monitor '%s'.", mon->name); + } + return handle; } @@ -216,7 +220,7 @@ stopMonitor(void *arg) GALERA_MONITOR *handle = (GALERA_MONITOR *) mon->handle; handle->shutdown = 1; - thread_wait((void *) handle->tid); + thread_wait(handle->thread); } /** diff --git a/server/modules/monitor/galeramon.h b/server/modules/monitor/galeramon.h index d4cf3b94c..54a63383c 100644 --- a/server/modules/monitor/galeramon.h +++ b/server/modules/monitor/galeramon.h @@ -51,7 +51,7 @@ typedef struct { SPINLOCK lock; /**< The monitor spinlock */ - pthread_t tid; /**< id of monitor thread */ + THREAD thread; /**< Monitor thread */ int shutdown; /**< Flag to shutdown the monitor thread */ int status; /**< Monitor status */ unsigned long id; /**< Monitor ID */ diff --git a/server/modules/monitor/mmmon.c b/server/modules/monitor/mmmon.c index e03fb396f..4cadf2ef5 100644 --- a/server/modules/monitor/mmmon.c +++ b/server/modules/monitor/mmmon.c @@ -173,7 +173,12 @@ startMonitor(void *arg, void* opt) { memset(handle->events, true, sizeof(handle->events)); } - handle->tid = (THREAD) thread_start(monitorMain, mon); + + if (thread_start(handle->thread, monitorMain, mon) == NULL) + { + MXS_ERROR("Failed to start monitor thread for monitor '%s'.", mon->name); + } + return handle; } @@ -189,7 +194,7 @@ stopMonitor(void *arg) MM_MONITOR *handle = (MM_MONITOR *) mon->handle; handle->shutdown = 1; - thread_wait((void *) handle->tid); + thread_wait((void *) handle->thread); } /** diff --git a/server/modules/monitor/mmmon.h b/server/modules/monitor/mmmon.h index 923a4b182..56e6c951e 100644 --- a/server/modules/monitor/mmmon.h +++ b/server/modules/monitor/mmmon.h @@ -44,7 +44,7 @@ typedef struct { SPINLOCK lock; /**< The monitor spinlock */ - pthread_t tid; /**< id of monitor thread */ + THREAD thread; /**< Monitor thread */ int shutdown; /**< Flag to shutdown the monitor thread */ int status; /**< Monitor status */ unsigned long id; /**< Monitor ID */ diff --git a/server/modules/monitor/mysql_mon.c b/server/modules/monitor/mysql_mon.c index aa5d5b19f..a9e8a13af 100644 --- a/server/modules/monitor/mysql_mon.c +++ b/server/modules/monitor/mysql_mon.c @@ -205,7 +205,11 @@ startMonitor(void *arg, void* opt) memset(handle->events, true, sizeof(handle->events)); } - handle->tid = (THREAD) thread_start(monitorMain, monitor); + if (thread_start(&handle->thread, monitorMain, monitor) == NULL) + { + MXS_ERROR("Failed to start monitor thread for monitor '%s'.", monitor->name); + } + return handle; } @@ -221,7 +225,7 @@ stopMonitor(void *arg) MYSQL_MONITOR *handle = (MYSQL_MONITOR *) mon->handle; handle->shutdown = 1; - thread_wait((void *) handle->tid); + thread_wait(handle->thread); } /** diff --git a/server/modules/monitor/mysqlmon.h b/server/modules/monitor/mysqlmon.h index b23877aa8..d0fef8406 100644 --- a/server/modules/monitor/mysqlmon.h +++ b/server/modules/monitor/mysqlmon.h @@ -60,7 +60,7 @@ typedef struct { SPINLOCK lock; /**< The monitor spinlock */ - pthread_t tid; /**< id of monitor thread */ + THREAD thread; /**< Monitor thread */ int shutdown; /**< Flag to shutdown the monitor thread */ int status; /**< Monitor status */ unsigned long id; /**< Monitor ID */ diff --git a/server/modules/monitor/ndbclustermon.c b/server/modules/monitor/ndbclustermon.c index 023bf2550..11d82909e 100644 --- a/server/modules/monitor/ndbclustermon.c +++ b/server/modules/monitor/ndbclustermon.c @@ -164,7 +164,12 @@ startMonitor(void *arg, void* opt) { memset(handle->events, true, sizeof(handle->events)); } - handle->tid = (THREAD) thread_start(monitorMain, mon); + + if (thread_start(&handle->thread, monitorMain, mon) == NULL) + { + MXS_ERROR("Failed to start monitor thread for monitor '%s'.", mon->name); + } + return handle; } @@ -180,7 +185,7 @@ stopMonitor(void *arg) MYSQL_MONITOR *handle = (MYSQL_MONITOR *) mon->handle; handle->shutdown = 1; - thread_wait((void *) handle->tid); + thread_wait(handle->thread); } /**