On Dec 18, 2007 2:03 PM, Sebastian Harl <<a href="mailto:sh@tokkee.org" target="_blank">sh@tokkee.org</a>> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Hi Justo,</blockquote><div>Hi Sebastian,<br> attach a new file with all your comments (I hope !!) ;-) <br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Just a few quick notes on your code after a first, quick glance:
<br><br>On Tue, Dec 18, 2007 at 12:45:08PM +0100, Justo Alonso wrote:<br>> static size_t tomcat_curl_callback (void *buf, size_t size, size_t nmemb, void *stream)<br>> {<br>> size_t len = size * nmemb;<br>
> char *new_buffer = NULL;
<br>><br>> /* maybe curl call funtion on last read and no bytes ... ?!?!?!? */<br>> if( len == 0 )<br>> return(0);<br>><br>> if( (new_buffer = (char *) realloc( tomcat_curl_buffer, tomcat_curl_buffer_len + len ) ) == NULL )
<br>> return -1;<br><br>Doing a lot of free()/malloc()'s frequently might lead to somewhat poor<br>performance if using glibc. Thus I suggest doing something like:<br><br>1) Introduce a new variable "tomcat_curl_buffer_fill" (or similar) indicating
<br> how much memory is currently used in the buffer.<br><br>2) Realloc if the buffer is too small:<br><br> if (tomcat_curl_buffer_len < tomcat_curl_buffer_fill + len)<br> new_buffer = realloc ...<br> ...<br>
<br>
3) In tomcat_read(): skip sfree() and simply reset tomcat_curL_buffer_fill to<br> zero.<br><br>Does that sound reasonable to you?</blockquote><div><br>Yes, I need to refresh my algorithms knowleges ;-)<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br><br>> ERROR ("tomcat: unable to create dinamic instance's list. [%s]", ci->values[0].value.string );<br><br>The correct spelling is "dynamic" instead of "dinamic".<br></blockquote>
<div><br>And use google translator too ... If you want, change the error/debug messages to be in a correct english<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>> if( tomcat_instances == NULL && ( ( tomcat_instances = llist_create() ) == NULL ) ) {<br><br>Just a small nitpick about coding style:<br><br>Please use one space after "if", "while", etc. keywords and function names and
<br>skip spaces after opening braces and before closing braces. Operators should<br>always be surrounded by a single space on both sides. E.g.:<br><br> if ((tomcat_instances == NULL)<br> && ((tomcat_instances = llist_craete()) == NULL)) {
<br> ...<br><br>Most importantly: try to be consistent - you've mixed several different coding<br>styles within your code which makes it kinda hard to read.<br></blockquote><div><br>It's ok now (I think) ... cut & paste source files ... ;-)
<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>> for( instance = llist_head(tomcat_instances); instance; instance = instance->next ) {
<br><br>Please use explicit conditional expressions: "instance != NULL" rather than<br>"instance".<br></blockquote><div><br>ok<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<br>> gc_name =attr->children == NULL ? NULL : attr->children->content;<br><br>Imho this should be written as follows to increase readability:<br><br> gc_name = (attr->children == NULL) ? NULL : attr->children->content;
<br><br>The missing space after "=" and the missing braces make it kinda hard to see<br>what's supposed to be going on.<br><div></div></blockquote><div><br>ok<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><br>> static int tomcat_read (void)<br>> {<br></div>> llentry_t *instance = NULL;<br>><br>> xmlDocPtr doc;<br>> xmlNode *root_element = NULL;<br>><br>> int success = 0;
<br>><br>> for( instance = llist_head(tomcat_instances); instance; instance = instance->next ) {<br>> tomcat_instance_t *ti = (tomcat_instance_t *) instance->value;<br>><br>> if( ti == NULL ) {
<br>> ERROR ("tomcat: read, instance object is null for instance %s", instance->key);<br>> return (-1);<br>> }<br>><br>> if (ti->curl == NULL)
<br>> return (-1);<br>> if (ti->url == NULL)<br>> return (-1);<br><br>Is it really appropriate to return an error in this case? It seems like this<br>
is not supposed to happen at all as those values should be set correctly when
<br>configuring the plugin, right? In this case, assert() would imho be more<br>appropriate, else "continue" should be used.</blockquote><div><br>It's ok. I change this check to init function (a instance should have a url parameter)
<br> <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br><br>Else the code looks fine to me :-)</blockquote><div><br>fine, but hard to read it !! ;-)
<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br><br>Cheers,<br>Sebastian<br><font color="#888888"></font></blockquote><div>
<br>j <br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><font color="#888888"><br>--<br>Sebastian "tokkee" Harl +++ GnuPG-ID: 0x8501C7FC +++
<a href="http://tokkee.org/" target="_blank">http://tokkee.org/</a><br><br>Those who would give up Essential Liberty to purchase a little Temporary<br>Safety, deserve neither Liberty nor Safety. -- Benjamin Franklin
<br><br></font><br>-----BEGIN PGP SIGNATURE-----<br>Version: GnuPG v1.4.6 (GNU/Linux)<br><br>iD8DBQFHZ8UXEFEKc4UBx/wRAv6fAJ9Hpm1ZDDpSA7BUQ+Nqr0kFz6/fqgCfb0WM<br>R6cgyqNccRt3xFlyrtoPN30=<br>=MsgA<br>-----END PGP SIGNATURE-----
<br><br>_______________________________________________<br>collectd mailing list<br><a href="mailto:collectd@verplant.org" target="_blank">collectd@verplant.org</a><br><a href="http://mailman.verplant.org/listinfo/collectd" target="_blank">
http://mailman.verplant.org/listinfo/collectd</a><br><br></blockquote></div><br>