[PATCH] new feature: automatic services for hosts

[Andreas Ericsson]

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.
> 
> What does it do?
> 
> The patch adds a new directive "services" to the "host" definitions.
> This new directive lists one or more "service" templates which
> are used to automatically create "service" definitions for
> the host.
> 
> The following sequence:
> 
>     define host{
>         host_name         bogus
>         address           192.168.1.254
>         services          template-check-disk-sda
>         ...
>     }
> 
> is exactly equivalent to the following sequence:
> 
>     define host{
>         host_name         bogus
>         address           192.168.1.254
>         ...
>     }
>     define service {
>         host_name         bogus
>         use               template-check-disk-sda
>     }
> 
> I don't particularly like the location where the automatically
> created services are instantiated. If someone has a better idea,
> including how to deal with host templates, I'm all ears.
> May be it could be handled better as an step before resolving.
> 

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.

> diff --git a/xdata/xodtemplate.c b/xdata/xodtemplate.c
> index be40c80..9eed89f 100644
> --- a/xdata/xodtemplate.c
> +++ b/xdata/xodtemplate.c
> @@ -124,6 +124,13 @@ char *xodtemplate_precache_file=NULL;
>  
>  int presorted_objects=FALSE;
>  
> +/*
> + * 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.

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

>  	if(!strcmp(input,"service"))
>  		xodtemplate_current_object_type=XODTEMPLATE_SERVICE;
> @@ -3665,6 +3675,14 @@ int xodtemplate_add_object_property(char *input, int options){
>  			temp_host->retain_nonstatus_information=(atoi(value)>0)?TRUE:FALSE;
>  			temp_host->have_retain_nonstatus_information=TRUE;
>  		        }
> +		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.

>  		else if(!strcmp(variable,"register"))
>  			temp_host->register_object=(atoi(value)>0)?TRUE:FALSE;
>  		else if(variable[0]=='_'){
> @@ -4707,6 +4725,87 @@ int xodtemplate_add_object_property(char *input, int options){
>  int xodtemplate_end_object_definition(int options){
>  	int result=OK;
>  
> +	/* Special case: if we just finished a "host", auto-generate
> +	 * any requested services as if we had also read:
> +	 *
> +	 *	define service {
> +	 *	    host_name HOSTNAME
> +	 *	    use TEMPLATE
> +	 *	}
> +	 *
> +	 * for each service name in the "services" statement.
> +	 */
> +	if ( ( xodtemplate_current_object_type == XODTEMPLATE_HOST ) &&
> +	     ( xodtemplate_host_auto_services != NULL ) ) {
> +		xodtemplate_host *host = (xodtemplate_host *) xodtemplate_current_object;
> +		char *servicelist = xodtemplate_host_auto_services;
> +		char *service;
> +
> +		/* Make sure that this is a true, honest-to-god, definition and
> +		 * not a template.
> +		 * We can't deal with this directive from a template (FIXME)
> +		 */
> +		if ( host->register_object ) {
> +
> +			/* Finish the object */
> +			xodtemplate_current_object=NULL;
> +			xodtemplate_current_object_type=XODTEMPLATE_NONE;
> +
> +			while ( ( result == OK ) &&
> +				( ( service = my_strsep( &servicelist, "," ) ) != NULL ) ) {
> +
> +				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().

> +				strip( service );
> +
> +				if ( service[0] == '\x0' ) {
> +					continue;
> +					}
> +
> +				/* Start */
> +				result = xodtemplate_begin_object_definition("service", options, xodtemplate_current_config_file, host->_start_line );
> +				if ( result != OK ) {
> +					break;
> +					}
> +
> +				/* "host_name HOSTNAME" */
> +				asprintf( &line, "host_name %s", host->host_name );
> +				result = xodtemplate_add_object_property( line, options );
> +				my_free( line );
> +				if ( result != OK ) {
> +					break;
> +					}
> +
> +				/* "use SERVICE_TEMPLATE" */
> +				asprintf( &line, "use %s", service );
> +				result = xodtemplate_add_object_property( line, options );
> +				my_free( line );
> +				if ( result != OK ) {
> +					break;
> +					}
> +
> +				/* end the service */
> +				result = xodtemplate_end_object_definition( options );
> +				if ( result != OK ) {
> +					break;
> +					}
> +				}
> +			}
> +
> +#ifdef NSCORE
> +		if ( result != OK ) {
> +			logit(NSLOG_CONFIG_ERROR,TRUE,
> +			      "Error: Could not add auto service definition '%s' for host '%s' in file '%s' on line %d.\n",
> +			      service,
> +			      host->host_name,
> +			      xodtemplate_config_file_name( host->_config_file ),
> +			      host->_start_line );
> +			}
> +#endif
> +
> +		my_free( xodtemplate_host_auto_services );
> +		xodtemplate_host_auto_services = NULL;
> +		}
>  

This too is an unnecessary explicit NULL-assignment.

Apart from that, it looks good.

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

Thanks for the patch. Good work. :)

-- 
Andreas Ericsson                   andreas.ericsson at op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

-------------------------------------------------------------------------
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=/