[collectd] Tomcat plugin

[Justo Alonso]

On Dec 18, 2007 2:03 PM, Sebastian Harl <sh at tokkee.org> wrote:

> Hi Justo,

Hi Sebastian,
   attach a new file with all your comments (I hope !!)  ;-)

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?


Yes, I need to refresh my algorithms knowleges ;-)


>
> >       ERROR ("tomcat: unable to create dinamic instance's list. [%s]",
> ci->values[0].value.string );
>
> The correct spelling is "dynamic" instead of "dinamic".
>

And use google translator too ...  If you want, change the error/debug
messages to be in a correct english


> >   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.
>

It's ok now (I think) ... cut & paste source files ... ;-)


>
> >       for( instance = llist_head(tomcat_instances); instance; instance =
> instance->next ) {
>
> Please use explicit conditional expressions: "instance != NULL" rather
> than
> "instance".
>

ok


>
> >               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.
>

ok


>
> > 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.


It's ok. I change this  check to init function (a instance should have a url
parameter)


>
>
> Else the code looks fine to me :-)


fine, but hard to read it !! ;-)


>
> Cheers,
> Sebastian
>

j

>
> --
> 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
>
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
>
> iD8DBQFHZ8UXEFEKc4UBx/wRAv6fAJ9Hpm1ZDDpSA7BUQ+Nqr0kFz6/fqgCfb0WM
> R6cgyqNccRt3xFlyrtoPN30=
> =MsgA
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> collectd mailing list
> collectd at verplant.org
> http://mailman.verplant.org/listinfo/collectd
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mailman.verplant.org/pipermail/collectd/attachments/20071218/51a0f844/attachment-0001.htm 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: tomcat.c
Type: text/x-csrc
Size: 18062 bytes
Desc: not available
Url : http://mailman.verplant.org/pipermail/collectd/attachments/20071218/51a0f844/attachment-0001.c