[PATCH] new feature: automatic services for hosts

[Ignacio Goyret]

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 code,
a couple of extra mallocs are not going to break the bank.

But, as a scientist working on identifying and fixing code
security issues, I have a real problem with allocating
buffers like that which can be easily overflown with a
malicious definition. A buffer overflow is the beginning
of most security attacks nowadays.

Hopefully this makes sense. If you still disagree,
you are welcome to adjust the code to your liking.


>Apart from that, it looks good.

Again, thanks for your through review.

Unfortunately, a couple of bugs crept up in that patch
as I was porting and cleaning the patch from my production
version. A new version of the patch is in the works.


>We'll see what Ethan thinks of breaking config syntax between releases
>though. Hopefully, this is deemed nice enough to warrant a 3.1-release
>(especially if 'services' support is added to hostgroups as well).

Mmm, adding this into hostgroups is a very interesting idea
which would benefit greatly with your other idea of concating
definitions.

If concating support was added (to this or any other statement),
consider adding a method to remove an element from the list,
in case a service is not applicable to a particular host but
is generally applicable to most hosts in the hostgroup.

The other issue to consider with concating is the handling
of duplicates, where the same service template is specified
in more than one hostgroups.

Cheers,
-Ignacio


-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/