Matches as an array (4.0)

[bazsi]

This builds on top of #3888 and implements $* as a list and other match related improvements.

These are the new features:

• $* is a new macro that generates a list from $1, $2, $3 ... name-value pairs
• the number of such name value pairs are tracked internally. Any new match operation (e.g. regexp matching) would reset the entire match array, while previously "stale" values could remain, if the new match operation would generate less matches than the previous one
• added set-matches() and unset-matches() rewrite operations, the first expecting a list, while the second just resets the matches array to be empty.

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build FAILURE

[bazsi]

@kira-syslogng test this please;

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[gaborznagy]

Just a note from our discussion: investigate the possibility of unsetting the stale/obsolete matches from earlier (i.e. when we have fewer matches in a subsequent matching than before).

[bazsi]

Just a note from our discussion: investigate the possibility of unsetting the stale/obsolete matches from earlier (i.e. when we have fewer matches in a subsequent matching than before).

It seems that I have implemented this already and then forgot about it. It is this patch: c67006b63d05d60530f1a435750b423281935ffe

This means that if we set non-consecutive match values then anything between the old range and the new one are unset explicitly.

I've now noticed that there's a function to reset the number of matches: log_msg_clear_matches() so I've committed changes to
use that function instead of open-coding the same.

[kira-syslogng]

Build SUCCESS

[gaborznagy]

I've checked it, and it looks good to me!

I have only some minor remarks about commit history, about a unit test, and some general remarks.

I don't have a better, more general alternative to the term "matches" in set-matches or unset-matches.
I'm not sure of their general nature either: users of $1 or set-matches() should be careful of regex capture groups that could overwrite their explicitly set matches.

In Bash, $*, $1, etc. all refer to positional parameters.
In Perl it was related to multiline matching in regular expressions, but not since Perl 5.10.

[gaborznagy]

I've sent a PR about the news entries.

[kira-syslogng]

Build FAILURE

[bazsi]

@kira-syslogng retest this please;

[kira-syslogng]

Build FAILURE

[gaborznagy]

@bazsi Kira fails due to the git security patch to CVE-2022-24765 (https://github.blog/2022-04-12-git-security-vulnerability-announced/)

[gaborznagy]

We'll be adding the suggested safe.directory git config workaround as I've had some test issues running the docker image as the repository's owner (some journald tests fail)

[bazsi]

Thanks @gaborznagy for the good feedback. I've now addressed all your review notes and pushed out another iteration. There seems to be some style-check related issue in the CI about a missing Python command. Otherwise things seem to be green (unless I am breaking something with the last force-push :)

Reviewing this one would be appreciated.

[kira-syslogng]

Build SUCCESS

[gaborznagy]

Thanks for resolving my comments.

[gaborznagy]

@bazsi this needs one last rebase to fix the Light style-check issue in Github actions.

[OverOrion]

log {
  source(s_src);
  rewrite {
    set("foo bar" value('1'));
    unset-matches();
  };
  destination { file("/dev/stdout" template("$(format-json --scope nv-pairs --key 1)\n")); };
};

With this patch we can still query the $1 value after the unset-matches(); is it an intended behaviour?
I would argue that after unset-matches() is called every $n should be empty (""), or at least that is what I expect.

[bazsi]

Good catch. The log_msg_values_foreach() did not filter out out-of-range matches. I'm pushing a fix for this, however I am starting to feel that it'd be much better to explicitly unset all matches explitictly, rather than trying to track the number of matches and then checking their number in all the code paths that query name-value pairs.

Might even be faster doing it only once, instead of at every log_msg_values_foreach() iteration.

[bazsi]

This is the fix for this: e234146b1ae44af0cdfff06e65917d40fa14c275

[gaborznagy]

I don't want to be nitpicking especially since I've approved this PR once, but this fix in e234146 only fixes the case when a cleared match is referenced through value-pairs, e.g. through format-json.

If the match (e.g. $1) is used later as an input in a parser or rewrite rule, then it would still hold the previously (cleared) value.
Maybe this scenario is rather a programming error, e.g. the writer of an SCL block should be aware of this.
A dummy example SCL:

parser example-parser() {
....
  set("foo bar" value('1'));
  unset-matches();
  syslog-parser(template('$1'));
};

TLDR: how about using log_msg_unset_match() instead of log_msg_clear_matches()?

[bazsi]

In the last iteration, I have changed the approach of resetting values, so the branch now contains a patch that changes things - again.

With that said, this case has worked correctly before, '$1' was unset because of this branch in log_msg_get_match():

+const gchar *
+log_msg_get_match_if_set_with_type(const LogMessage *self, gint index_, gssize *value_len,
+                                   LogMessageValueType *type)
+{
+  g_assert(index_ >= 0 && index_ < LOGMSG_MAX_MATCHES);
+
+  if (index_ >= self->num_matches)
+    return NULL;
+
+  return nv_table_get_value_if_set(self->payload, match_handles[index_], value_len, type);
+}
+

e.g. if a match number was outside the range of num_matches we always considered it to be unset (NOTE the NULL return). This worked correctly even before. The value-pairs code path had a bug as @OverOrion noticed correctly, as it didn't use log_msg_get_match(), rather it iterated over all name-value pairs using log_msg_values_foreach(), which did not filter out $1 in this case. That's what my last patch fixed.

With all this iterations however, at the end I've decided to go back to the original concept and call log_msg_unset() all matches whenever the num_matches would change. This eliminates the need to handle matches separately from other name-value pairs in the query APIs.

This is this patch that reverses most of the above.

commit 855e0c4174ebbba5db05d9395a2d33efb6b1cc4f
Author: Balazs Scheidler bazsi77@gmail.com
Date: Tue May 3 13:48:12 2022 +0200

logmsg: unset matches proactively

With that we are failing the cisco-parser() testcase in light, so I am now going to find that issue.

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[bazsi]

I have found yet another case that clobbers the borrowed input of LogMatcher instances, which caused the change in
#3934 to fail. Added some more unit tests and a fix too.

[kira-syslogng]

Build SUCCESS

[OverOrion]

LGTM!

Also thank your for not only addressing the review notes, but writing additional tests for those as well!

[gaborznagy]

regexp-parser() still uses log_matcher_match directly instead of the new wrappers (buffer, value, and template).
I've checked it and it's fine this way: it doesn't have to use log_matcher_match_value, as the NVTable reference counting happens in LogParser layer, can't leverage the advantages of log_matcher_match_template either, as the template is formatted already in the LogParser layer.

Maybe log_matcher_match_buffer could be used to emphasize that the caller already provided the functionalities and requirements supplied by those wrappers otherwise.

[gaborznagy]

Approved. This was a very long time and lot of efforts for all of us, but I think it really worth it.
Sorry for taking it so long. :)

Otherwise I would ask to squash some patches (especially that some feature took opposite directions), but it would require a lot of effort again from all of us.
The patches are self-contained (not fixup patches), so we can merge it in this state.