[collectd] Unixsock plugin memleaks
Gerrie Roos
groos at xiplink.com
Wed May 30 22:30:14 CEST 2012
Hi Dave
Yay, so I've found some leaks and prepared a diff! I've also included misc.
other fixes and one cool feature. I'm still working on 4.10.2 so the
patches might not work or still be applicable to 5.x, but last time I
checked at least some of the issues I fixed were not fixed in 5.x. I assume
you guys will review and manually merge into 5.x and/or 4.10.x. I'd be
happy to provide more info and answer questions if anything I've done is not
clear.
Quick overview:
Fixed various collectd memory leaks.
Fixed collectd's unixsock read interrupted by SIGCHLD's.
Fixed collectd exec module: incorrectly exited select loop on signal.
Fixed incorrect collectd warning when configuring network plugin.
Collectd's scale plugin now supports scaling only on specific data
sources, yay! (New feature)
Apologies if I wasn't supposed to paste the patch in the body of the email,
but here goes:
commit ba04be9feeec4f40b9ada1cae64c0a27634f1c76
Author: Gerrie Roos <groos at xiplink.com>
Date: Fri May 25 05:38:47 2012 +0200
Fixed various collectd memory leaks.
Once I understood what's going on I tried to keep the changes to a
minimum.
diff --git a/src/collectd.c b/src/collectd.c
index 277d3b0..f5df460 100644
--- a/src/collectd.c
+++ b/src/collectd.c
@@ -77,6 +77,7 @@ static void sig_usr1_handler (int __attribute__((unused))
signal)
pthread_attr_init (&attr);
pthread_attr_setdetachstate (&attr, PTHREAD_CREATE_DETACHED);
pthread_create (&thread, &attr, do_flush, NULL);
+ pthread_attr_destroy (&attr);
}
static int init_hostname (void)
diff --git a/src/exec.c b/src/exec.c
index 322ff67..265ec01 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -827,6 +827,7 @@ static int exec_read (void) /* {{{ */
pthread_attr_init (&attr);
pthread_attr_setdetachstate (&attr, PTHREAD_CREATE_DETACHED);
pthread_create (&t, &attr, exec_read_one, (void *) pl);
+ pthread_attr_destroy(&attr);
} /* for (pl) */
return (0);
@@ -870,6 +871,7 @@ static int exec_notification (const notification_t *n,
/* {{{ */
pthread_attr_init (&attr);
pthread_attr_setdetachstate (&attr, PTHREAD_CREATE_DETACHED);
pthread_create (&t, &attr, exec_notification_one, (void *) pln);
+ pthread_attr_destroy (&attr);
} /* for (pl) */
return (0);
diff --git a/src/meta_data.c b/src/meta_data.c
index 6a336c4..9307e5e 100644
--- a/src/meta_data.c
+++ b/src/meta_data.c
@@ -214,6 +214,7 @@ void meta_data_destroy (meta_data_t *md) /* {{{ */
if (md == NULL)
return;
+ pthread_mutex_destroy(&md->lock);
md_entry_free (md->head);
free (md);
} /* }}} void meta_data_destroy */
diff --git a/src/unixsock.c b/src/unixsock.c
index 1010b59..dea8be0 100644
--- a/src/unixsock.c
+++ b/src/unixsock.c
@@ -187,6 +187,7 @@ static void *us_handle_client (void *arg)
close (fdin);
close (fdout);
pthread_exit ((void *) 1);
+ return ((void *) 0);
}
fhout = fdopen (fdout, "w");
@@ -198,6 +199,7 @@ static void *us_handle_client (void *arg)
fclose (fhin); /* this closes fdin as well */
close (fdout);
pthread_exit ((void *) 1);
+ return ((void *) 0);
}
/* change output buffer to line buffered mode */
@@ -209,6 +211,7 @@ static void *us_handle_client (void *arg)
fclose (fhin);
fclose (fhout);
pthread_exit ((void *) 1);
+ return ((void *) 0);
}
while (42)
@@ -253,6 +256,7 @@ static void *us_handle_client (void *arg)
fclose (fhin);
fclose (fhout);
pthread_exit ((void *) 1);
+ return ((void *) 0);
}
if (strcasecmp (fields[0], "getval") == 0)
@@ -307,9 +311,13 @@ static void *us_server_thread (void
__attribute__((unused)) *arg)
pthread_t th;
pthread_attr_t th_attr;
+ pthread_attr_init (&th_attr);
+ pthread_attr_setdetachstate (&th_attr, PTHREAD_CREATE_DETACHED);
+
if (us_open_socket () != 0)
pthread_exit ((void *) 1);
+
while (loop != 0)
{
DEBUG ("unixsock plugin: Calling accept..");
@@ -325,6 +333,7 @@ static void *us_server_thread (void
__attribute__((unused)) *arg)
sstrerror (errno, errbuf, sizeof (errbuf)));
close (sock_fd);
sock_fd = -1;
+ pthread_attr_destroy(&th_attr);
pthread_exit ((void *) 1);
}
@@ -341,9 +350,6 @@ static void *us_server_thread (void
__attribute__((unused)) *arg)
DEBUG ("Spawning child to handle connection on fd #%i", *remote_fd);
- pthread_attr_init (&th_attr);
- pthread_attr_setdetachstate (&th_attr, PTHREAD_CREATE_DETACHED);
-
status = pthread_create (&th, &th_attr, us_handle_client, (void *)
remote_fd);
if (status != 0)
{
@@ -358,6 +364,7 @@ static void *us_server_thread (void
__attribute__((unused)) *arg)
close (sock_fd);
sock_fd = -1;
+ pthread_attr_destroy(&th_attr);
status = unlink ((sock_file != NULL) ? sock_file : US_DEFAULT_PATH);
if (status != 0)
diff --git a/src/utils_cmd_listval.c b/src/utils_cmd_listval.c
index 4ca9646..f923fca 100644
--- a/src/utils_cmd_listval.c
+++ b/src/utils_cmd_listval.c
@@ -89,8 +89,9 @@ int handle_listval (FILE *fh, char *buffer)
print_to_socket (fh, "%i Value%s found\n",
(int) number, (number == 1) ? "" : "s");
- for (i = 0; i < number; i++)
+ for (i = 0; i < number; i++) {
print_to_socket (fh, "%u %s\n", (unsigned int) times[i], names[i]);
+ }
free_everything_and_return (0);
} /* int handle_listval */
diff --git a/src/utils_fbhash.c b/src/utils_fbhash.c
index d20b7e3..9f1058b 100644
--- a/src/utils_fbhash.c
+++ b/src/utils_fbhash.c
@@ -234,6 +234,7 @@ void fbh_destroy (fbhash_t *h) /* {{{ */
if (h == NULL)
return;
+ pthread_mutex_destroy(&h->lock);
free (h->filename);
fbh_free_tree (h->tree);
} /* }}} void fbh_destroy */
commit 402e0a1746a67e47619ffc4ce2ad6317e8884cfe
Author: Gerrie Roos <groos at xiplink.com>
Date: Tue Apr 17 15:22:36 2012 +0200
Fixed collectd's unixsock read interrupted by SIGCHLD's.
diff --git a/src/unixsock.c b/src/unixsock.c
index 0b89748..1010b59 100644
--- a/src/unixsock.c
+++ b/src/unixsock.c
@@ -222,6 +222,9 @@ static void *us_handle_client (void *arg)
errno = 0;
if (fgets (buffer, sizeof (buffer), fhin) == NULL)
{
+ if (errno == EINTR)
+ continue;
+
if (errno != 0)
{
char errbuf[1024];
commit 9f4c960f94bc70d680400b50a84655f288e7e96d
Author: Gerrie Roos <groos at xiplink.com>
Date: Tue Jan 10 09:00:42 2012 +0200
Fixed collectd exec module: incorrectly exited select loop on signal.
diff --git a/src/exec.c b/src/exec.c
index c64f949..322ff67 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -584,8 +584,15 @@ static void *exec_read_one (void *arg) /* {{{ */
/* We use a copy of fdset, as select modifies it */
copy = fdset;
- while (select(highest_fd + 1, ©, NULL, NULL, NULL ) > 0)
+ while (1)
{
+ int e = select(highest_fd + 1, ©, NULL, NULL, NULL );
+ if (e < 0) {
+ if (errno == EINTR)
+ continue;
+ break;
+ }
+
int len;
if (FD_ISSET(fd, ©))
commit 27dd489e1242af63d2df04dfe627c440512e8da1
Author: Gerrie Roos <groos at xiplink.com>
Date: Tue Dec 20 06:30:59 2011 +0200
Fixed incorrect collectd warning when configuring network plugin.
diff --git a/src/network.c b/src/network.c
index 412b457..aa1c858 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1685,9 +1685,9 @@ static int network_set_interface (const sockent_t *se,
const struct addrinfo *ai
}
/* else: Not a multicast interface. */
-#if defined(HAVE_IF_INDEXTONAME) && HAVE_IF_INDEXTONAME &&
defined(SO_BINDTODEVICE)
if (se->interface != 0)
{
+#if defined(HAVE_IF_INDEXTONAME) && HAVE_IF_INDEXTONAME &&
defined(SO_BINDTODEVICE)
char interface_name[IFNAMSIZ];
if (if_indextoname (se->interface, interface_name) == NULL)
@@ -1704,11 +1704,10 @@ static int network_set_interface (const sockent_t
*se, const struct addrinfo *ai
sstrerror (errno, errbuf, sizeof (errbuf)));
return (-1);
}
- }
/* #endif HAVE_IF_INDEXTONAME && SO_BINDTODEVICE */
#else
- WARNING ("network plugin: Cannot set the interface on a unicast "
+ WARNING ("network plugin: Cannot set the interface on a unicast "
"socket because "
# if !defined(SO_BINDTODEVICE)
"the the \"SO_BINDTODEVICE\" socket option "
@@ -1718,6 +1717,8 @@ static int network_set_interface (const sockent_t *se,
const struct addrinfo *ai
"is not available on your system.");
#endif
+ }
+
return (0);
} /* }}} network_set_interface */
commit 641dc2dd4c595ede24c3997e4aa9db5c4528a2cd
Author: Gerrie Roos <groos at xiplink.com>
Date: Wed May 25 18:28:55 2011 +0200
Collectd's scale plugin now supports scaling only on specific data
sources, yay!
diff --git a/src/target_scale.c b/src/target_scale.c
index 29fecdf..a8d7038 100644
--- a/src/target_scale.c
+++ b/src/target_scale.c
@@ -29,6 +29,9 @@ struct ts_data_s
{
double factor;
double offset;
+
+ char **data_sources;
+ size_t data_sources_num;
};
typedef struct ts_data_s ts_data_t;
@@ -300,6 +303,67 @@ static int ts_config_set_double (double *ret,
oconfig_item_t *ci) /* {{{ */
return (0);
} /* }}} int ts_config_set_double */
+static int ts_config_add_data_source(ts_data_t *data, /* {{{ */
+ oconfig_item_t *ci)
+{
+ size_t new_data_sources_num;
+ char **temp;
+ int i;
+
+ /* Check number of arbuments. */
+ if (ci->values_num < 1)
+ {
+ ERROR ("`value' match: `%s' needs at least one argument.",
+ ci->key);
+ return (-1);
+ }
+
+ /* Check type of arguments */
+ for (i = 0; i < ci->values_num; i++)
+ {
+ if (ci->values[i].type == OCONFIG_TYPE_STRING)
+ continue;
+
+ ERROR ("`value' match: `%s' accepts only string arguments "
+ "(argument %i is a %s).",
+ ci->key, i + 1,
+ (ci->values[i].type == OCONFIG_TYPE_BOOLEAN)
+ ? "truth value" : "number");
+ return (-1);
+ }
+
+ /* Allocate space for the char pointers */
+ new_data_sources_num = data->data_sources_num + ((size_t) ci->values_num);
+ temp = (char **) realloc (data->data_sources,
+ new_data_sources_num * sizeof (char *));
+ if (temp == NULL)
+ {
+ ERROR ("`value' match: realloc failed.");
+ return (-1);
+ }
+ data->data_sources = temp;
+
+ /* Copy the strings, allocating memory as needed. */
+ for (i = 0; i < ci->values_num; i++)
+ {
+ size_t j;
+
+ /* If we get here, there better be memory for us to write to. */
+ assert (data->data_sources_num < new_data_sources_num);
+
+ j = data->data_sources_num;
+ data->data_sources[j] = sstrdup (ci->values[i].value.string);
+ if (data->data_sources[j] == NULL)
+ {
+ ERROR ("`value' match: sstrdup failed.");
+ continue;
+ }
+ data->data_sources_num++;
+ }
+
+ return (0);
+} /* }}} int ts_config_add_data_source */
+
static int ts_destroy (void **user_data) /* {{{ */
{
ts_data_t **data;
@@ -309,6 +373,13 @@ static int ts_destroy (void **user_data) /* {{{ */
data = (ts_data_t **) user_data;
+ if (*data && (*data)->data_sources) {
+ size_t i;
+ for (i = 0; i < (*data)->data_sources_num; i++)
+ free((*data)->data_sources[i]);
+ free((*data)->data_sources);
+ }
+
free (*data);
*data = NULL;
@@ -341,6 +412,8 @@ static int ts_create (const oconfig_item_t *ci, void
**user_data) /* {{{ */
status = ts_config_set_double (&data->factor, child);
else if (strcasecmp ("Offset", child->key) == 0)
status = ts_config_set_double (&data->offset, child);
+ else if (strcasecmp ("DataSource", child->key) == 0)
+ status = ts_config_add_data_source(data, child);
else
{
ERROR ("Target `scale': The `%s' configuration option is not understood
"
@@ -393,6 +466,18 @@ static int ts_invoke (const data_set_t *ds,
value_list_t *vl, /* {{{ */
for (i = 0; i < ds->ds_num; i++)
{
+ /* If we've got a list of data sources, is it in the list? */
+ if (data->data_sources) {
+ size_t j;
+ for (j = 0; j < data->data_sources_num; j++)
+ if (strcasecmp(ds->ds[i].name, data->data_sources[j]) == 0)
+ break;
+
+ /* No match, ignore */
+ if (j >= data->data_sources_num)
+ continue;
+ }
+
if (ds->ds[i].type == DS_TYPE_COUNTER)
ts_invoke_counter (ds, vl, data, i);
else if (ds->ds[i].type == DS_TYPE_GAUGE)
Hope you find this useful,
gerrie
_____
From: David Halko [mailto:davidhalko at gmail.com]
Sent: 25 May 2012 09:23 PM
To: groos at xiplink.com
Cc: collectd at verplant.org
Subject: Re: [collectd] Unixsock plugin memleaks
It sounds like a good deal, if you share the changed,
if you have fixed some memory leaks!!!
Dave
http://netmgt.blogspot.com/
On Thu, May 24, 2012 at 11:21 AM, Gerrie Roos <groos at xiplink.com> wrote:
Hi ppl
I've been chasing some memory leaks in the unixsock plugin (already found a
couple) but there might still be some leaks. Has anybody been working on
this plugin recently? I'd be happy to share what I've got so far.
gerrie
_______________________________________________
collectd mailing list
collectd at verplant.org
http://mailman.verplant.org/listinfo/collectd
_____
No virus found in this message.
Checked by AVG - www.avg.com
Version: 2012.0.2176 / Virus Database: 2425/5022 - Release Date: 05/25/12
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.verplant.org/pipermail/collectd/attachments/20120530/8be661e1/attachment-0001.html>
More information about the collectd
mailing list