From: Florian Forster Date: Sat, 19 Nov 2016 13:26:27 +0000 (+0100) Subject: src/daemon/plugin.c: Address review comments. X-Git-Tag: collectd-5.7.0~32^2 X-Git-Url: https://git.octo.it/?p=collectd.git;a=commitdiff_plain;h=e81567f2a645fd15f4384ca9569119bc66c1412a src/daemon/plugin.c: Address review comments. --- diff --git a/configure.ac b/configure.ac index 9c5df839..49baf0a3 100644 --- a/configure.ac +++ b/configure.ac @@ -214,7 +214,54 @@ AC_HEADER_SYS_WAIT AC_HEADER_DIRENT AC_HEADER_STDBOOL -AC_CHECK_HEADERS(stdio.h errno.h math.h stdarg.h syslog.h fcntl.h signal.h assert.h sys/types.h sys/socket.h sys/select.h poll.h netdb.h arpa/inet.h sys/resource.h sys/param.h kstat.h regex.h sys/ioctl.h endian.h sys/isa_defs.h fnmatch.h libgen.h pthread_np.h) +AC_CHECK_HEADERS([ \ + arpa/inet.h \ + assert.h \ + ctype.h \ + endian.h \ + errno.h \ + fcntl.h \ + fnmatch.h \ + fs_info.h \ + fshelp.h \ + grp.h \ + kstat.h \ + kvm.h \ + libgen.h \ + limits.h \ + locale.h \ + math.h \ + mntent.h \ + mnttab.h \ + netdb.h \ + paths.h \ + poll.h \ + pthread_np.h \ + pwd.h \ + regex.h \ + signal.h \ + stdarg.h \ + stdio.h \ + sys/fs_types.h \ + sys/fstyp.h \ + sys/ioctl.h \ + sys/isa_defs.h \ + sys/mntent.h \ + sys/mnttab.h \ + sys/param.h \ + sys/resource.h \ + sys/select.h \ + sys/socket.h \ + sys/statfs.h \ + sys/statvfs.h \ + sys/types.h \ + sys/un.h \ + sys/vfs.h \ + sys/vfstab.h \ + sys/vmmeter.h \ + syslog.h \ + wordexp.h \ +]) # For entropy plugin on newer NetBSD AC_CHECK_HEADERS(sys/rndio.h, [], [], @@ -658,31 +705,6 @@ AC_CHECK_HEADERS(linux/un.h, [], [], #endif ]) -AC_CHECK_HEADERS([ \ - ctype.h \ - fs_info.h \ - fshelp.h \ - grp.h \ - kvm.h \ - limits.h \ - locale.h \ - mntent.h \ - mnttab.h \ - paths.h \ - pwd.h \ - sys/fs_types.h \ - sys/fstyp.h \ - sys/mntent.h \ - sys/mnttab.h \ - sys/statfs.h \ - sys/statvfs.h \ - sys/un.h \ - sys/vfs.h \ - sys/vfstab.h \ - sys/vmmeter.h \ - wordexp.h \ -]) - # --enable-xfs {{{ AC_ARG_ENABLE([xfs], [AS_HELP_STRING([--enable-xfs], [xfs support in df plugin @<:@default=yes@:>@])], @@ -1666,7 +1688,7 @@ AC_CHECK_MEMBERS([kstat_io_t.nwritten, kstat_io_t.writes, kstat_io_t.nwrites, ks SAVE_LDFLAGS="$LDFLAGS" LDFLAGS="$LDFLAGS -lpthread" -AC_MSG_CHECKING([if have pthread_setname_np]) +AC_MSG_CHECKING([for pthread_setname_np]) have_pthread_setname_np="no" AC_LINK_IFELSE([AC_LANG_PROGRAM( [[ @@ -1684,7 +1706,7 @@ AC_MSG_CHECKING([if have pthread_setname_np]) AC_MSG_RESULT([$have_pthread_setname_np]) # check for pthread_set_name_np(3) (FreeBSD) -AC_MSG_CHECKING([if have pthread_set_name_np]) +AC_MSG_CHECKING([for pthread_set_name_np]) have_pthread_set_name_np="no" AC_LINK_IFELSE([AC_LANG_PROGRAM( [[ diff --git a/src/daemon/plugin.c b/src/daemon/plugin.c index ac9ce5bd..d54abeea 100644 --- a/src/daemon/plugin.c +++ b/src/daemon/plugin.c @@ -123,7 +123,7 @@ static int read_loop = 1; static pthread_mutex_t read_lock = PTHREAD_MUTEX_INITIALIZER; static pthread_cond_t read_cond = PTHREAD_COND_INITIALIZER; static pthread_t *read_threads = NULL; -static int read_threads_num = 0; +static size_t read_threads_num = 0; static cdtime_t max_read_interval = DEFAULT_MAX_READ_INTERVAL; static write_queue_t *write_queue_head; @@ -651,7 +651,38 @@ static void *plugin_read_thread (void __attribute__((unused)) *args) return ((void *) 0); } /* void *plugin_read_thread */ -static void start_read_threads (int num) +#ifdef PTHREAD_MAX_NAMELEN_NP +# define THREAD_NAME_MAX PTHREAD_MAX_NAMELEN_NP +#else +# define THREAD_NAME_MAX 16 +#endif + +static void set_thread_name(pthread_t tid, char const *name) { +#if defined(HAVE_PTHREAD_SETNAME_NP) || defined(HAVE_PTHREAD_SET_NAME_NP) + + /* glibc limits the length of the name and fails if the passed string + * is too long, so we truncate it here. */ + char n[THREAD_NAME_MAX]; + if (strlen (name) >= THREAD_NAME_MAX) + WARNING("set_thread_name(\"%s\"): name too long", name); + sstrncpy (n, name, sizeof(n)); + +#if defined(HAVE_PTHREAD_SETNAME_NP) + int status = pthread_setname_np (tid, n); + if (status != 0) + { + char errbuf[1024]; + ERROR ("set_thread_name(\"%s\"): %s", n, + sstrerror (status, errbuf, sizeof(errbuf))); + } +#else /* if defined(HAVE_PTHREAD_SET_NAME_NP) */ + pthread_set_name_np (tid, n); +#endif + +#endif +} + +static void start_read_threads (size_t num) /* {{{ */ { if (read_threads != NULL) return; @@ -664,39 +695,35 @@ static void start_read_threads (int num) } read_threads_num = 0; - for (int i = 0; i < num; i++) + for (size_t i = 0; i < num; i++) { - if (pthread_create (read_threads + read_threads_num, NULL, - plugin_read_thread, NULL) == 0) - { -#if defined(HAVE_PTHREAD_SETNAME_NP) || defined(HAVE_PTHREAD_SET_NAME_NP) - char thread_name[32]; - ssnprintf(thread_name, sizeof (thread_name), - "plugin reader#%d", i); -# if defined(HAVE_PTHREAD_SETNAME_NP) - pthread_setname_np (*(read_threads + read_threads_num), - thread_name); -# elif defined(HAVE_PTHREAD_SET_NAME_NP) - pthread_set_name_np (*(read_threads + read_threads_num), - thread_name); -# endif -#endif - read_threads_num++; - } - else + int status = pthread_create (read_threads + read_threads_num, + /* attr = */ NULL, + plugin_read_thread, + /* arg = */ NULL); + if (status != 0) { - ERROR ("plugin: start_read_threads: pthread_create failed."); + char errbuf[1024]; + ERROR ("plugin: start_read_threads: pthread_create failed " + "with status %i (%s).", status, + sstrerror (status, errbuf, sizeof (errbuf))); return; } + + char name[THREAD_NAME_MAX]; + ssnprintf (name, sizeof (name), "reader#%zu", read_threads_num); + set_thread_name (read_threads[read_threads_num], name); + + read_threads_num++; } /* for (i) */ -} /* void start_read_threads */ +} /* }}} void start_read_threads */ static void stop_read_threads (void) { if (read_threads == NULL) return; - INFO ("collectd: Stopping %i read threads.", read_threads_num); + INFO ("collectd: Stopping %zu read threads.", read_threads_num); pthread_mutex_lock (&read_lock); read_loop = 0; @@ -704,7 +731,7 @@ static void stop_read_threads (void) pthread_cond_broadcast (&read_cond); pthread_mutex_unlock (&read_lock); - for (int i = 0; i < read_threads_num; i++) + for (size_t i = 0; i < read_threads_num; i++) { if (pthread_join (read_threads[i], NULL) != 0) { @@ -892,9 +919,7 @@ static void start_write_threads (size_t num) /* {{{ */ write_threads_num = 0; for (size_t i = 0; i < num; i++) { - int status; - - status = pthread_create (write_threads + write_threads_num, + int status = pthread_create (write_threads + write_threads_num, /* attr = */ NULL, plugin_write_thread, /* arg = */ NULL); @@ -905,20 +930,13 @@ static void start_write_threads (size_t num) /* {{{ */ "with status %i (%s).", status, sstrerror (status, errbuf, sizeof (errbuf))); return; - } else { -#if defined(HAVE_PTHREAD_SETNAME_NP) || defined(HAVE_PTHREAD_SET_NAME_NP) - char thread_name[16]; - sstrncpy (thread_name, "plugin writer", sizeof(thread_name)); -# if defined(HAVE_PTHREAD_SETNAME_NP) - pthread_setname_np (*(write_threads + write_threads_num), - thread_name); -# elif defined(HAVE_PTHREAD_SET_NAME_NP) - pthread_set_name_np (*(write_threads + write_threads_num), - thread_name); -# endif -#endif - write_threads_num++; } + + char name[THREAD_NAME_MAX]; + ssnprintf (name, sizeof (name), "writer#%zu", write_threads_num); + set_thread_name (write_threads[write_threads_num], name); + + write_threads_num++; } /* for (i) */ } /* }}} void start_write_threads */ @@ -1807,7 +1825,7 @@ int plugin_init_all (void) rt = global_option_get ("ReadThreads"); num = atoi (rt); if (num != -1) - start_read_threads ((num > 0) ? num : 5); + start_read_threads ((num > 0) ? ((size_t) num) : 5); } return ret; } /* void plugin_init_all */ @@ -2819,35 +2837,30 @@ static void *plugin_thread_start (void *arg) } /* void *plugin_thread_start */ int plugin_thread_create (pthread_t *thread, const pthread_attr_t *attr, - void *(*start_routine) (void *), void *arg, char *name) + void *(*start_routine) (void *), void *arg, char const *name) { plugin_thread_t *plugin_thread; - int ret; plugin_thread = malloc (sizeof (*plugin_thread)); if (plugin_thread == NULL) - return -1; + return ENOMEM; plugin_thread->ctx = plugin_get_ctx (); plugin_thread->start_routine = start_routine; plugin_thread->arg = arg; - ret = pthread_create (thread, attr, + int ret = pthread_create (thread, attr, plugin_thread_start, plugin_thread); - - if (ret == 0 && name != NULL) { -#if defined(HAVE_PTHREAD_SETNAME_NP) || defined(HAVE_PTHREAD_SET_NAME_NP) - char thread_name[16]; - sstrncpy (thread_name, name, sizeof(thread_name)); -# if defined(HAVE_PTHREAD_SETNAME_NP) - pthread_setname_np (*thread, thread_name); -# elif defined(HAVE_PTHREAD_SET_NAME_NP) - pthread_set_name_np (*thread, thread_name); -# endif -#endif + if (ret != 0) + { + sfree (plugin_thread); + return ret; } - return ret; + if (name != NULL) + set_thread_name (*thread, name); + + return 0; } /* int plugin_thread_create */ /* vim: set sw=8 ts=8 noet fdm=marker : */ diff --git a/src/daemon/plugin.h b/src/daemon/plugin.h index 0f4d267c..9a9bf497 100644 --- a/src/daemon/plugin.h +++ b/src/daemon/plugin.h @@ -465,7 +465,7 @@ cdtime_t plugin_get_interval (void); */ int plugin_thread_create (pthread_t *thread, const pthread_attr_t *attr, - void *(*start_routine) (void *), void *arg, char *name); + void *(*start_routine) (void *), void *arg, char const *name); /* * Plugins need to implement this diff --git a/src/network.c b/src/network.c index 6b43ae75..d7eb32c3 100644 --- a/src/network.c +++ b/src/network.c @@ -3477,7 +3477,7 @@ static int network_init (void) status = plugin_thread_create (&dispatch_thread_id, NULL /* no attributes */, dispatch_thread, - NULL /* no argument */, "network dispatch"); + NULL /* no argument */, "network disp"); if (status != 0) { char errbuf[1024]; diff --git a/src/unixsock.c b/src/unixsock.c index 4a7652ab..2df4e096 100644 --- a/src/unixsock.c +++ b/src/unixsock.c @@ -378,7 +378,7 @@ static void *us_server_thread (void __attribute__((unused)) *arg) DEBUG ("Spawning child to handle connection on fd #%i", *remote_fd); status = plugin_thread_create (&th, &th_attr, - us_handle_client, (void *) remote_fd, "unixsock client"); + us_handle_client, (void *) remote_fd, "unixsock conn"); if (status != 0) { char errbuf[1024]; @@ -459,7 +459,7 @@ static int us_init (void) loop = 1; status = plugin_thread_create (&listen_thread, NULL, - us_server_thread, NULL, "unixsock listener"); + us_server_thread, NULL, "unixsock listen"); if (status != 0) { char errbuf[1024];