[collectd] Tomcat plugin

[Sebastian Harl]

Hi Justo,

Just a few quick notes on your code after a first, quick glance:

On Tue, Dec 18, 2007 at 12:45:08PM +0100, Justo Alonso wrote:
> static size_t tomcat_curl_callback (void *buf, size_t size, size_t nmemb, void *stream)
> {
> 	size_t len = size * nmemb;
> 	char *new_buffer = NULL;
> 
> 	/* maybe curl call funtion on last read and no bytes ... ?!?!?!? */
> 	if( len == 0 )
> 		return(0);
> 
> 	if( (new_buffer = (char *) realloc( tomcat_curl_buffer, tomcat_curl_buffer_len + len ) ) == NULL )
> 		return -1;

Doing a lot of free()/malloc()'s frequently might lead to somewhat poor
performance if using glibc. Thus I suggest doing something like:

1) Introduce a new variable "tomcat_curl_buffer_fill" (or similar) indicating
   how much memory is currently used in the buffer.

2) Realloc if the buffer is too small:

  if (tomcat_curl_buffer_len < tomcat_curl_buffer_fill + len)
      new_buffer = realloc ...
  ...

3) In tomcat_read(): skip sfree() and simply reset tomcat_curL_buffer_fill to
   zero.

Does that sound reasonable to you?
 
> 	ERROR ("tomcat: unable to create dinamic instance's list. [%s]", ci->values[0].value.string );

The correct spelling is "dynamic" instead of "dinamic".

>   if( tomcat_instances == NULL && ( ( tomcat_instances = llist_create() ) == NULL ) ) {

Just a small nitpick about coding style:

Please use one space after "if", "while", etc. keywords and function names and
skip spaces after opening braces and before closing braces. Operators should
always be surrounded by a single space on both sides. E.g.:

  if ((tomcat_instances == NULL)
          && ((tomcat_instances = llist_craete()) == NULL)) {
      ...

Most importantly: try to be consistent - you've mixed several different coding
styles within your code which makes it kinda hard to read.

> 	for( instance = llist_head(tomcat_instances); instance; instance = instance->next ) {

Please use explicit conditional expressions: "instance != NULL" rather than
"instance".

> 		gc_name =attr->children == NULL ? NULL : attr->children->content;

Imho this should be written as follows to increase readability:

  gc_name = (attr->children == NULL) ? NULL : attr->children->content;

The missing space after "=" and the missing braces make it kinda hard to see
what's supposed to be going on.

> static int tomcat_read (void)
> {
> 	llentry_t *instance = NULL;
> 
> 	xmlDocPtr doc;
> 	xmlNode *root_element = NULL;
> 
> 	int success = 0;
> 
> 	for( instance = llist_head(tomcat_instances); instance; instance = instance->next ) {
> 		tomcat_instance_t *ti = (tomcat_instance_t *) instance->value;
> 
> 		if( ti == NULL ) {
> 			ERROR ("tomcat: read, instance object is null for instance %s", instance->key);
> 			return (-1);
> 		}
> 
> 		if (ti->curl == NULL)
> 			return (-1);
> 		if (ti->url == NULL)
> 			return (-1);

Is it really appropriate to return an error in this case? It seems like this
is not supposed to happen at all as those values should be set correctly when
configuring the plugin, right? In this case, assert() would imho be more
appropriate, else "continue" should be used.

Else the code looks fine to me :-)

Cheers,
Sebastian

-- 
Sebastian "tokkee" Harl +++ GnuPG-ID: 0x8501C7FC +++ http://tokkee.org/

Those who would give up Essential Liberty to purchase a little Temporary
Safety, deserve neither Liberty nor Safety.         -- Benjamin Franklin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://mailman.verplant.org/pipermail/collectd/attachments/20071218/4fe8794e/attachment.pgp