Discussed a bit in ZBX-10590.

I think for now we need to fail creation of such calculated items like we do for regular items after ZBX-10723.

Similar problem was in ZBX-10828 for item key parsing. The solution for that problem was not to parse such macro.
Example
Key:

{{echo.sh[{$MACRO}]}}

Macro:

 a\ 

Result:

echo.sh[{$MACRO}]

Fixed in development branch svn://svn.zabbix.com/branches/dev/ZBX-11223. Quoted parameters can't end with a backslash and now LLD rules don't create items if it's syntax is wrong.

(1) That's not your fault, but comment regarding quote_key_param() return codes does not seem to be right. Or at least FAIL part, parameter ending with backslash does not always result in failure.

s.paskevics RESOLVED in r62870, r62879.

glebs.ivanovskis Thank you! Slightly improved (hopefully) wording in r63326. Have a look. Still RESOLVED

s.paskevics Looks good.
CLOSED

(2) Let's add more examples resulting in FAIL to the comment of substitute_key_macros() (I mean macros containing "]" and ",", not just leading whitespace). Is it possible to squeeze the comment into usual comment width by shrinking "macro" column?

s.paskevics RESOLVED in r62879.

glebs.ivanovskis Looks good, reordered and added few more examples in r63325. Please have a look. Still RESOLVED.

s.paskevics Looks good.
CLOSED

(3) Currently making of LLD calculated item formula looks like so:

1. find function and all its parameters in formula prototype
2. for each parameter:
   • unquote it
   • substitute LLD macros in it
3. for each parameter:
   • quote it
   • write it into LLD item formula

And these two loops are separated into different functions (substitute_formula_macros() and zbx_function_tostr()) and even different files. This is not convenient. Also we do double work of parsing with zbx_function_parse() and function_parse_name()/function_parse_param(). Would be nice to put everything in one place. It will eliminate the need of passing quoted in zbx_function_t.

s.paskevics RESOLVED in r62870.

glebs.ivanovskis Seems that workflow is the same and double work is still there. Am I looking in the wrong place?

s.paskevics I refactored zbx_function_tostr() function. Now this function quites and converts from f_function_t to string without double parsing. Function zbx_function_parse() is called only once from substitute_formula_macros()

glebs.ivanovskis I revised a whole function parsing ecosystem in r63411 and r63419. Quick summary:

1. Optimized out one of function_parse_name() parameters because it was redundant.
2. Condensed all quoted/unquoted parameter parsing logic into one function and made it with extern visibility. zbx_function_param_parse() will operate on a pre-validated list of parameters instead of looking for ')' in formula expression which needs validation. So it is now suitable for parsing trigger function parameters for trigger evaluation.
3. Added zbx_no_function() to skip function-less parts of formula, suitable for both calculated items and prototypes. This will be the place where we can put more complex logic of skipping user macros together with contexts and and-or-not operators preceding '(' in 3.0 and newer versions.
4. Added match_parenthesis() which will validate in-formula function parameters.
5. Added zbx_function_params_find() on top of zbx_no_function() and match_parenthesis(). It allows to parse formula in large chunks minimizing number of needed function calls.
6. Rewritten substitute_formula_macros(), now all unquoting, macro substitution and quoting parameters back is done there. Easy to observe.
7. Removed zbx_function_tostr() hence there is no need for it.
8. Removed zbx_function_t, zbx_function_clean() and zbx_function_parse(). We might want to resurrect them and use as a replacement for old parse_function().

Still RESOLVED

glebs.ivanovskis Further improvements in r63466. Now calculated item evaluation uses the same parser as calculated item LLD. Still RESOLVED

s.paskevics

1. if first function parameter contain LLD macro then substitute_formula_macros parse incorrectly.
2. evaluate function works correctly only with expressions without white spaces. See my fix c63494
3. Need to update comments for following functions - get_trigger_function_value, evaluate_macro_function, zbx_function_param_unquote_dyn
4. About this check in get_trigger_function_value:

   if (SUCCEED != zbx_function_find(p, &f_pos, &par_l, &par_r) || 0 != f_pos || '}' != p[par_r + 1])
     goto fail;
   

   I think better to move 0 != f_pos inside zbx_function_find
   REOPENED

glebs.ivanovskis This subissue is getting difficult to manage:

1. moved to (9);
2. moved to (10);
3. moved to (11);
4. moved to (12).

Let's consider this one
CLOSED

(4) What's dynamic about function_quote_param_dyn() now? Can we drop _dyn?
Oh, it allocates storage for out, puts quoted parameter in there and leaves it there? Something is wrong here.

s.paskevics RESOLVED in r62870.

glebs.ivanovskis You forgot that if parameter was quoted initially it needs to get quoted back. Currently only "value-induced" quoting occurs.
REOPENED

s.paskevics I changed the code. How quoting should work properly. RESOLVED in r63012.

glebs.ivanovskis Good!
CLOSED

(5) Parsing and quoting rules for item key parameters and function parameters are very similar ("]" -> ")" is the only difference). Can we adapt quote_key_param() and unquote_key_param() to work for both keys and functions and throw function_quote_param_dyn() away?

The only caveat here is that quote_key_param() is used in DB upgrades. We must be careful.

glebs.ivanovskis There is a difference in forced quoting rules not reflected in quote_key_param() currently. See ZBX-11281. But anyway, function_quote_param_dyn() should use the same design pattern as quote_key_param().

s.paskevics RESOLVED in r62870.

glebs.ivanovskis Looks good! The only thing remaining is to pass parameter string and quoted/unquoted state as separate parameters.
REOPENED

s.paskevics The function parameter is zbx_func_param_t. quoted/unquoted flag already exists in this structure. Should it be changed?

glebs.ivanovskis What I actually meant is that this function is in str.c where we mostly have functions working with strings, not complex structures. Anyway, see what happened to it in r63419.
RESOLVED

s.paskevics
CLOSED

(6) By the way, do we feel like fixing ZBX-11281 within this ticket?

s.paskevics Please check it. RESOLVED in r63012.

glebs.ivanovskis Opening bracket is treated as array beginning only if it is the first character or is preceded by spaces. So we don't need to check the whole parameter for '[', just the very first character. Leading space will force quoting on its own. New case when we force item key parameter quoting deserves a mention in substitute_key_macros() comment. Would be nice to check the result of parse_item_key() in get_value_simple() as well. This would save us from crashes in case of corrupted database and finally would make Coverity happy. REOPENED

s.paskevics RESOLVED in r63408.

glebs.ivanovskis Exactly what we need! Added my $0.02 in r63426.
CLOSED

(7) Shouldn't we fail in substitute_formula_macros() if function has no parameters or if parse_host_key() returns FAIL according to "fail as soon as possible" rule?

glebs.ivanovskis According to sasha, server should not validate formula syntax during discovery rule processing, frontend should validate calculated item prototype formula instead. Story for another issue.
WON'T FIX

(8) Found new error, if macros value is empty and item or function parameter is quoted (for example - key("{$MACRO}") then zabbix tries to read data outside of the array.
Function quote_key_param in sysinfo.c:

sz_src = strlen(*param); //sz_src = 0

if ('\\' == (*param)[sz_src - 1]) // error
        return FAIL;

Warning message from valgrind:

==16616== Invalid read of size 1
==16616==    at 0x44EEC9: quote_key_param (sysinfo.c:1075)
==16616==    by 0x462614: replace_key_param_cb (expression.c:4810)
==16616==    by 0x49515B: replace_key_param (str.c:1616)
==16616==    by 0x4955DC: replace_key_params_dyn (str.c:1759)
==16616==    by 0x45BA06: substitute_key_macros (expression.c:4863)
==16616==    by 0x42C610: get_values (poller.c:398)
==16616==    by 0x42D837: poller_thread (poller.c:707)

s.paskevics RESOLVED. See r63451.

glebs.ivanovskis Looks good!
CLOSED

(9) Moved from (3.1):

s.paskevics If first function parameter contain LLD macro then substitute_formula_macros() parse incorrectly.

glebs.ivanovskis What happens there is that first parameter cannot be parsed as <host:>key and macro substitution fails for the whole formula. Although I admit that error "Cannot create item: Invalid item key at position 0." is way too cryptic. Attempted to improve error message in r63546.
RESOLVED

s.paskevics
CLOSED

(10) Moved from (3.2):

s.paskevics Evaluate function works correctly only with expressions without white spaces. See my fix c63494
RESOLVED

glebs.ivanovskis Please see my correction in r63496, should be fine now.
Still RESOLVED

s.paskevics
CLOSED

(11) Moved from (3.3):

s.paskevics Need to update comments for following functions:

• get_trigger_function_value(),
• evaluate_macro_function(),
• zbx_function_param_unquote_dyn().

glebs.ivanovskis RESOLVED in r63558.

s.paskevics
CLOSED

(12) Moved from (3.4):

s.paskevics About this check in get_trigger_function_value():

if (SUCCEED != zbx_function_find(p, &f_pos, &par_l, &par_r) || 0 != f_pos || '}' != p[par_r + 1])
  goto fail;

I think better to move 0 != f_pos inside zbx_function_find()

glebs.ivanovskis I agree. Your solution in r63533 looked like we try to squeeze two functions into one body, so I decided to make two functions.
RESOLVED in r63546.

s.paskevics Looks good.
CLOSED

(13) Thank you that you noticed that leading whitespace counting problem.
RESOLVED in r63552.

s.paskevics
CLOSED

(14) Let's move parse_host_key() call and related code from calcitem_evaluate_expression() to calcitem_parse_expression(). The latter will then resemble substitute_formula_macros() and we won't need to remove_param() explicitly.

s.paskevics RESOLVED in r63557.

glebs.ivanovskis Compilation warnings:

checks_calculated.c: In function ‘calcitem_parse_expression’:
checks_calculated.c:92:34: warning: unused variable ‘nparam’ [-Wunused-variable]
  int  functionid, ret = SUCCEED, nparam = 0, quoted;
                                  ^
checks_calculated.c: In function ‘calcitem_evaluate_expression’:
checks_calculated.c:262:1: warning: label ‘out’ defined but not used [-Wunused-label]
 out:
 ^

s.paskevics RESOLVED in r63561.

glebs.ivanovskis A few flaws in your implementation:

1. memory allocation associated with func and params will be leaked in case parse_host_key() fails;
2. error message ""Invalid first parameter in function ..." will not show parameter in question because it's stored in buf, not in params;
3. if there is one and the only function parameter, everything past the ')' will be copied into params.

See my fixes in r63579.
Still RESOLVED

s.paskevics
CLOSED

(15) Suspicious code:

	if (i == items->values_num)	/* no item found */
	{
		...
		item->snmp_oid = zbx_strdup(NULL, snmp_oid_proto);
		item->snmp_oid_orig = NULL;
		if (ITEM_TYPE_SNMPv1 == type || ITEM_TYPE_SNMPv2c == type || ITEM_TYPE_SNMPv3 == type)
		{
			if (FAIL == substitute_key_macros(&item->snmp_oid, NULL, NULL, jp_row, MACRO_TYPE_SNMP_OID,
					err, sizeof(err)))
			{
				goto out;
			}

			zbx_lrtrim(item->snmp_oid, ZBX_WHITESPACE);
		}
		...
	}
	else
	{
		...
		if (ITEM_TYPE_SNMPv1 == type || ITEM_TYPE_SNMPv2c == type || ITEM_TYPE_SNMPv3 == type)
		{
			buffer = zbx_strdup(buffer, snmp_oid_proto);
			if (FAIL == substitute_key_macros(&buffer, NULL, NULL, jp_row, MACRO_TYPE_SNMP_OID,
					err, sizeof(err)))
			{
				goto out;
			}

			zbx_lrtrim(buffer, ZBX_WHITESPACE);
			if (0 != strcmp(item->snmp_oid, buffer))
			{
				item->snmp_oid_orig = item->snmp_oid;
				item->snmp_oid = buffer;
				buffer = NULL;
				item->flags |= ZBX_FLAG_LLD_ITEM_UPDATE_SNMP_OID;
			}
		}
		...
	}

Why do we zbx_strdup(..., snmp_oid_proto) outside of the if for new LLD items and inside the if for existing ones? snmp_oid_proto comes from the database every time we process LLD data, theoretically it may change from time to time. Shouldn't we do zbx_lrtrim(..., ZBX_WHITESPACE) outside of the if as well?

I understand, that it won't make any difference what exactly is stored in snmp_oid for non-SNMP items, but almost identical code can catch an eye of the reader and drive him crazy before it realizes that this subtle difference has no effect.

s.paskevics These checks are removed in r63575. This problem will be considered and resolved under different issue.

glebs.ivanovskis Nice! The rest
MOVED to ZBX-11434.

(16) In case of error in substitute_formula_macros() we leave '\0' in the original formula.
RESOLVED in r63572.

s.paskevics
CLOSED

Successfully tested. Let's merge!

Fixed in:

• pre-2.2.16rc1 r63589.
• pre-3.0.6rc1 r63620.
• pre-3.2.2rc1 r63638.
• pre-3.3.0 (trunk) r63649.

(17) Absence of error message about the failed item update from prototype (how it is done for item creation). Created new dev branch for this fix - svn://svn.zabbix.com/branches/dev/ZBX-11223-2

s.paskevics RESOLVED in r63655.

glebs.ivanovskis If item creation or update failed in two possible places (key and formula) shouldn't we show both error messages so that user can solve both issues before the next LLD run?
REOPENED

glebs.ivanovskis On update we show error message for every field we failed to update for some reason. On new item creation failure we show only the first error message. Still REOPENED, but before continuing on this subissue let's test and merge svn://svn.zabbix.com/branches/dev/ZBX-11223-3

s.paskevics Changes in svn://svn.zabbix.com/branches/dev/ZBX-11223-3 successfully tested.

glebs.ivanovskis Ported necessary changes to svn://svn.zabbix.com/branches/dev/ZBX-11223-2
RESOLVED in r64007.

s.paskevics Looks good.
CLOSED

(18) I guess it's not good to goto out; in the middle of item creation or update. We can leak memory or end up with half-updated item. Applicable to 2.2 as well.

glebs.ivanovskis Yes, we definitely leaked item when failed to create new item. As to half-updated items, we actually do such things. If only one field is invalid we fail to update that particular field and successfully update the others. Fixed for 2.2 in svn://svn.zabbix.com/branches/dev/ZBX-11223-3 r63845 and r63846.
RESOLVED

s.paskevics Successfully tested.
CLOSED

Note: a coding issue was found by Coverity. See ZBX-11453.

(19) Compilation warnings.
In 2.2 and 3.0:

lld_item.c: In function ‘substitute_formula_macros’:
lld_item.c:874:33: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   for (p = e + par_l + 1; p - e < par_r ; p += sep_pos + 1)
                                 ^
lld_item.c:891:14: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
    if (p - e == par_l + 1)
              ^

In 3.2 and later additionally to warnings above also this warning:

macrofunc.c: In function ‘zbx_calculate_macro_function’:
macrofunc.c:131:28: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
  for (ptr = buf; ptr - buf < buf_offset; ptr += sep_pos + 1)
                            ^

glebs.ivanovskis RESOLVED for 2.2 in svn://svn.zabbix.com/branches/dev/ZBX-11223-3 r63842

s.paskevics Successfully tested.
CLOSED

(18) and (19) fixed in:

• pre-2.2.16rc1 r63994.

(17), (18) and (19) fixed in:

• pre-3.0.6rc1 r64011;
• pre-3.2.2rc1 r64012;
• pre-3.3.0 (trunk) r64013.

Documented in:
What's new in Zabbix *:

• https://www.zabbix.com/documentation/2.2/manual/introduction/whatsnew2216
• https://www.zabbix.com/documentation/3.0/manual/introduction/whatsnew306
• https://www.zabbix.com/documentation/3.2/manual/introduction/whatsnew322

glebs.ivanovskis Wording "in case macros resolving is not correct" is a bit confusing in my opinion. I would say "in case macro resolving cannot be fully accomplished" or even "in case necessary item and function parameter quoting after macro resolving cannot be accomplished" for more details. Maybe martins-v and VSI have a better idea, let's consult them.

s.paskevics I think that the common phrase fits best ("in case macro resolving cannot be fully accomplished"), because there are several kind of errors (not only quoting), but main idea is that we have begun notify users that the LLD does not create incorrect items. glebs.ivanovskis please check my changes in documentation.

martins-v I've moved the entry together with daemon improvements, because it is not a major LLD improvement.

glebs.ivanovskis Thank you, martins-v! Looks good.

(20) [S] Another compilation warning in 3.3.

checks_calculated.c:166:50: warning: unused parameter 'dc_item' [-Wunused-parameter]
static int      calcitem_evaluate_expression(DC_ITEM *dc_item, expression_t *exp, char *error, size_t max_error_len,
                                                      ^
1 warning generated.

s.paskevics This warning is fixed in ZBX-11481. CLOSED