expand macros for non-existant custom variable as empty string

[klaernie]

I hope adding the changelog entry to the topmost entry is the right place.

[tsadpbb]

This looks to be the correct change. I'm curious what you think about having a similar logic for ALL macros? I'm curious if there are some negatives.

The reason I ask is that some commands might use a macro that is only available in certain contexts, like the how $CONTACTNAME$ is only available in notifications

[klaernie]

I would appreciate that behavior, but did not grok how that purpose differentiation works - or I might simply have overlooked it.

[everwatch]

If I expand a non-existent shell variable I get a blank. I think the same behavior inside of a Nagios script makes sense. It would be up to me, the developer, to make sure that the variable is defined before I use it. So I am in 100% agreement that any non-existent macro should expand to a blank.

[aaronagios]

What do we want to do here? This change seems to be welcomed. We could merge and be better off. @tsadpbb what would the effort look like to expand this to all macros? Would we want to offer an addition here or make a separate issue?

[tsadpbb]

It would involve modifying the following which can be found around line 195

/* a non-macro, just some user-defined string between two $s */
else {
	log_debug_info(DEBUGL_MACROS, 2, "  Non-macro.  Running output (%lu): '%s'\n", (unsigned long)strlen(*output_buffer), *output_buffer);

	/* add the plain text to the end of the already processed buffer */
	*output_buffer = (char *)realloc(*output_buffer, strlen(*output_buffer) + strlen(temp_buffer) + 3);
	strcat(*output_buffer, "$");
	strcat(*output_buffer, temp_buffer);

	/* just could have been a stray $ */
	if (buf_ptr != NULL) {
		strcat(*output_buffer, "$");
	}
}

[aaronagios]

If we made a change there does it remove the need for this PR/would it also address the custom variable issue?

[klaernie]

It is a little more complicated - one would want to ensure that each known prefix gets an "expand empty" shortcut, but the keeping unknown text that does not have one of the supported prefixes must not be replaced - else users could not put small inline bash scripts into macros, if they use $(…).

[tsadpbb]

You are right, upon reading some more of macros.c, I agree it is more complicated

[tsadpbb]

I noticed that when you have no custom variables defined for an object, this added logic is never met because vars is NULL when the object does not have any custom variables

Near line 2470

	if(macro_name == NULL || vars == NULL || output == NULL)
		return ERROR;

Used in a context like follows

result = grab_custom_object_macro_r(mac, macro_name + 5, temp_host->custom_variables, output);

I would suggest changing the check to

	if(macro_name == NULL || output == NULL)
		return ERROR;

Or doing something similar

Also update the changelog to add this to version 4.5.6

[aaronagios]

@klaernie we're releasing 4.5.6 tomorrow, it doesn't look like your change here will make that, but I would like to get it included in 4.5.7 next month. May I ask, what do you think of the notes above?

[klaernie]

Sorry, got swamped for a moment. I rebased on top of the current master branch, and incorporated the suggestion by @tsadpbb - thanks for catching that!

Just in case this PR still makes it, I moved the changelog entry to 4.5.6, but in case that does not happen it's no problem to move it again.

[aaronagios]

Hi @klaernie,

Thanks for the update! I'll merge this now and move the changelog to 4.5.7 after the merge.

Aaron

[klaernie]

Thanks, @aaronagios !