Nagios Support Forum

At 12:34 PM 11/21/2008 +0100, Andreas Ericsson wrote:
>Ignacio Goyret wrote:
>> Hi all,
>> The attached patch is something that I've been using for a while
>> without problems. Of course, all appropriate disclaimers apply.
>>
>
>This is nice, but with a twist. While I like the general
>direction of this patch, I think a couple of things should be
>changed about it. First off, indentation in Nagios doesn't
>add spaces neither before nor after parentheses. I'll fix
>that up when applying though. More comments inlined below.

Thanks for your review. Your feedback is very much appreciated!
Answers inline below.

>> +/*
>> + * List of service templates to be automatically instantiated for
>> + * the current host. This is used while reading a 'define host {}'
>> + * section.
>> + */
>> +char *xodtemplate_host_auto_services=NULL;
>
>This needn't be a global variable. If it *has* to be a global, it should
>be declared 'static' (as it isn't used outside this file). For 'static'
>vars, you don't have to initialize it explicitly to NULL.

Yes, of course. It should have been static all along.
I fell ill with cut-and-pastitis...

In the new verion of the patch which I'll submit soon, I've moved
this variable to xodtemplate_host_struct with the eventual intention
of handling templates correctly.



>> /******************************************************************/
>> /************* TOP-LEVEL CONFIG DATA INPUT FUNCTION ***************/
>> @@ -843,6 +850,9 @@ int xodtemplate_begin_object_definition(char *input, int options, int config_fil
>> xodtemplate_serviceextinfo *new_serviceextinfo=NULL;
>> register int x=0;
>>
>> + /* Used only by "host", but doesn't hurt to always initialize */
>> + my_free( xodtemplate_host_auto_services );
>> + xodtemplate_host_auto_services=NULL;
>>
>
>my_free() sets the targeted variable to NULL. Doing it explicitly means extra
>code. The compiler will optimize it away, but it's bad if someone else looks at
>how you did it for future patches. Besides, since you initialize it below
>anyways, it's unnecessary here.

Sorry for these ones. I thought I had caught all of them but
obviously, I didn't. This has been fixed.


>> + else if(!strcmp(variable,"services")){
>> + if(strcmp(value,XODTEMPLATE_NULL)){
>> + my_free( xodtemplate_host_auto_services );
>> + xodtemplate_host_auto_services = (char *)strdup(value);
>> + if ( xodtemplate_host_auto_services == NULL )
>> + result=ERROR;
>> + }
>> + }
>
>This should probably be done by appending the new value to
>xodtemplate_host_auto_services, so that multiple services can be specified
>by giving the "services" stanza several times. Although that would go against
>most other stanzas in Nagios ("use" being the exception), it's probably worth
>it for this time, as it can then be applied to hostgroups too in a sane way.

That's an interesting idea of concating definitions.
However, although it would very easy to implement, I'd rather
keep it in line with the behavior for all the other statements.

From the code I've seen, none of the statements allow multiple
inclusions within the same definition block (including "use").
In fact, if you do repeate a statement with a string value,
the first statement will be ignored silently and you will have
a memory leak (of the first value that was strdup'd).

FWIW, the "services" handler tries not to leak memory but it
still silently ignores previous definitions.



>> +
>> + char *line;
>> +
>
>If you make "char *line" a stack-allocated variable (char line[2048]) instead,
>you can get away with not using asprintf() below, which will severely fragment
>the memory arena, and instead rely on the much more widely available snprintf().

Sorry, I don't agree. Let me explain why.

Yes, I understand that fragmentation may be an issue,
but considering the number of strdup() all over the

...[email truncated]...


This post was automatically imported from historical nagios-devel mailing list archives
Original poster: [email protected]