On Wed, 2009-11-04 at 15:17 +0100, Michael Friedrich wrote:
> For the single free statements I fully agree with the implementation.
> But for the longer parts I would recommend another way of
> implementation - I think your direction is kind of complicated, no
> offense.
>
> /* generate the query */
> asprintf(&api_query,"v=1&product=icinga&tinycheck=1&stableonly=1");
> if(bare_update_check==FALSE)
> asprintf(&api_query,"%s&version=%s%s",api_query,PROGRAM_VERSION,(api_query_opts==NULL)?"":api_query_opts);
>
> rewritten to
>
> /* generate the query */
>
> if(bare_update_check==FALSE) {
> asprintf(&api_query,"%s&version=%s%s",api_query,PROGRAM_VERSION,(api_query_opts==NULL)?"":api_query_opts);
> } else {
> asprintf(&api_query,"v=1&product=icinga&tinycheck=1&stableonly=1");
> }
>
> does the same, but does not need a second buffer and needs less cycles
> as your proposal.
Actually, api_query hasn't been initialized up until that point. The
api_query pointer in your first asprintf is dependent on the results of
the asprintf in your else clause. If you wanted to rewrite it then it
would need to be more like:
if(bare_update_check==FALSE) {
asprintf(&api_query,"v=1&product=icinga&tinycheck=1&stableonly=1&version=%s%s",PROGRAM_VERSION,(api_query_opts==NULL)?"":api_query_opts);
} else {
asprintf(&api_query,"v=1&product=icinga&tinycheck=1&stableonly=1");
}
This does cause some duplication in the strings, but does otherwise look
a bit cleaner.
> same for
>
> asprintf(&buf,"POST %s HTTP/1.0\r\n",api_path);
> asprintf(&buf,"%sUser-Agent: Nagios/%s\r\n",buf,PROGRAM_VERSION);
> asprintf(&buf,"%sConnection: close\r\n",buf);
> asprintf(&buf,"%sHost: %s\r\n",buf,api_server);
> asprintf(&buf,"%sContent-Type: application/x-www-form-urlencoded\r\n",buf);
> asprintf(&buf,"%sContent-Length: %d\r\n",buf,strlen(api_query));
> asprintf(&buf,"%s\r\n",buf);
> asprintf(&buf,"%s%s\r\n",buf,api_query);
>
> if this causes a real memory leak, put it alltogether in one single
> line and buffer.
>
> asprintf(&buf,"POST %s HTTP/1.0\r\nUser-Agent: Nagios/%s\r\nConnection: close\r\nHost: %s\r\nContent-Type: application/x-www-form-urlencoded\r\nContent-Length: %d\r\n\r\n%s\r\n",api_path, PROGRAM_VERSION, api_server, strlen(api_query), api_query);
>
> if you need better reading then just reformat the string in the code a
> bit.
>
>
> Your opinions on that?
I think that is a fine solution and also much cleaner looking. I was
trying to keep changes to the style to a minimum, but it is often hard
to best know how to do that for a given project.
What I really wanted to do was define something like my_asprintf() that
worked something like (entirely untested):
int my_asprintf(char **strp, const char *fmt, ...) {
char *tmp = NULL;
int result;
va_list ap;
va_start(ap, fmt);
result = vasprintf(&tmp, fmt, ap);
va_end(ap);
if (*strp)
free(*strp);
*strp = tmp;
return result;
}
Which would hopefully allow the asprintf() style calls in the same
fashion as they had been used, except substituting my_asprintf() for
them.
However, I didn't see any source file which was shared in common across
all the Nagios source files that used asprintf and adding a new source
file started to leave the realm of small and simple patch which I was
aiming for (ugly as it was).
And, yes, the current code does cause a small memory leak. As far as I
can see, asprintf always malloc()s new memory and places the pointer in
the first argument without using the existing value (no realloc, free,
etc.) I found it while running valgrind on Nagios while pursuing a
different problem and figured it was worthy of a janitorial patch.
Thanks for the feedback.
Regards,
Sean
|
