basicfuncs: add list-search function

[alltilla]

Returns with the index of the first match of keyword in the list.
Starts the search at start_index. 0 based. If not found, returns empty string.

New usage:

• Literal match:
  $(list-search <pattern> ${list})
  or
  $(list-search --mode literal <pattern> ${list})
• Prefix match: $(list-search --mode prefix <pattern> ${list})
• Substring match: $(list-search --mode substring <pattern> ${list})
• Glob match: $(list-search --mode glob <pattern> ${list})
• Regex match: $(list-search --mode pcre <pattern> ${list})

Add --start-index <index> to change the start index.

Implements #2924

TODO:

• Add starting position
• Add backwards search
• Support regex, glob, etc...
• Support prefix, substring
• Cleanup commits

Signed-off-by: Attila Szakacs attila.szakacs@balabit.com

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[bazsi]

On Fri, Oct 4, 2019, 11:55 Kókai Péter ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In modules/basicfuncs/list-funcs.c <#2955 (comment)>: > @@ -24,6 +24,8 @@ #include "scanner/list-scanner/list-scanner.h" #include "str-repr/encode.h" +#include <pcre.h> Please use compat/pcre.h instead, that covers PCRE_STUDY_JIT_COMPILE and others. ------------------------------ In modules/basicfuncs/list-funcs.c <#2955 (comment)>: > @@ -70,6 +72,278 @@ tf_list_append(LogMessage *msg, gint argc, GString *argv[], GString *result) TEMPLATE_FUNCTION_SIMPLE(tf_list_append); +typedef struct _LSPattern +{ + GString *keyword; + GPatternSpec *glob; + GString *keyword_reversed; + pcre *regex; + pcre_extra *regex_extra; +} LSPattern; + +enum ListSearchMode +{ + LSM_LITERAL = 0, It feels like you reimplement the LogMatcher just working with list instead of LogMessage. [optional] would you take a look if the two could have the same root ?

I am all for this, just a note: LogMatcher is not a good base for this, I hate that code. So the new base layer should be created and then an improved LogMatcher layer put on top.

------------------------------

In modules/basicfuncs/tests/test_basicfuncs.c <#2955 (comment)>: > + assert_template_format("$(list-search)", ""); + assert_template_format("$(list-search almafa)", ""); + assert_template_format("$(list-search almafa 0)", ""); I would consider those as incorrect call, as searching nothing sure could mean nothing, but most likely an user error this is.

The template function infrastructure has a distinction over whether the user passed an argument which is empty over the user didnt pass an argument (via argc which denotes the number of arguments). I agree that missed arguments should be reported.

…

------------------------------

In modules/basicfuncs/list-funcs.c <#2955 (comment)>: > + const gchar *errptr; + gint erroffset; + gint rc; + + self->pcre = pcre_compile2(self->keyword->str, PCRE_ANCHORED, &rc, &errptr, &erroffset, NULL); + if (!self->pcre) + { + msg_error("Error while compiling regular expression", + evt_tag_str("regular_expression", self->keyword->str), + evt_tag_str("error_at", &self->keyword->str[erroffset]), + evt_tag_int("error_offset", erroffset), + evt_tag_str("error_message", errptr), + evt_tag_int("error_code", rc)); + return FALSE; + } + self->pcre_extra = pcre_study(self->pcre, 0, &errptr); Please use PCRE_STUDY_JIT_COMPILE to enable JIT whenever possible. ------------------------------ In modules/basicfuncs/list-funcs.c <#2955 (comment)>: > + +static void +_list_search(gint argc, GString *argv[], GString *result, ListSearchParameters *parameters) +{ + if (argc < 1) + return; + + ListScanner scanner; + gint index = -1; + + list_scanner_init(&scanner); + list_scanner_input_gstring_array(&scanner, argc, argv); + + while (list_scanner_scan_next(&scanner)) + { + if (++index < parameters->start_index) [optional] you could extract the skip part into a separate while wrapped with a function calling it like skip until start index. ------------------------------ In modules/basicfuncs/list-funcs.c <#2955 (comment)>: > +{ + if (argc < 1) + return; + + ListScanner scanner; + gint index = -1; + + list_scanner_init(&scanner); + list_scanner_input_gstring_array(&scanner, argc, argv); + + while (list_scanner_scan_next(&scanner)) + { + if (++index < parameters->start_index) + continue; + + if (_list_search_match(list_scanner_get_current_value_as_gstring(&scanner), parameters)) I am not fun of the get_as_type like functions. In this case I think the current list_scanner_get_current_value could return with GString, and the users could be updated accordingly. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2955?email_source=notifications&email_token=AAFOK5U3SPYETI3PHSPPH2DQM4HIHA5CNFSM4I4WYLHKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCG4SZBY#pullrequestreview-297348231>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFOK5RAJIGERLQWR3ZLRZLQM4HIHANCNFSM4I4WYLHA> .

[kira-syslogng]

Build SUCCESS

[alltilla]

@Kokan, @bazsi:
Yeah, this is I believe the 3rd implementation of pcre now :/
Should I do the refactor in this PR, or before/after it?

[kira-syslogng]

Build SUCCESS

[Kokan]

Because that refactor requires to change the LogMatcher that has effect on many modules/functionality; and the logic can be cut off from this change request I would do it in a different PR.
The current PR is imho okay to merge, as the list-search implementation only effects the new code.
You could resolve the "conflict" when one of them is merged.

Short: do it "later" / in parallel.

But I am fine with whatever schedule as well.

[gaborznagy]

Please change list-search to TEMPLATE_FUNCTION

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[gaborznagy]

Please fix the missing tf_simple_* calls and using the correct input for the scanner.

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[kira-syslogng]

Build SUCCESS

[gaborznagy]

Besides a minor improvement idea, approve!

[bazsi]

Right now it is a convention that list handling functions concatenate arguments as separate lists, which means you dont need to use list-append.

…

On Sat, Dec 7, 2019, 15:10 Gábor Nagy ***@***.***> wrote: ***@***.**** approved this pull request. Besides a minor improvement idea, approve! ------------------------------ In modules/basicfuncs/list-funcs.c <#2955 (comment)>: > + g_free(mode_str); + g_option_context_free(ctx); + + return result; +} + +static gboolean +_list_search_init_matcher(ListSearchState *state, StringMatchMode mode, gint argc, gchar *argv[], GError **error) +{ + if (argc < 2) + { + g_set_error(error, LOG_TEMPLATE_ERROR, LOG_TEMPLATE_ERROR_COMPILE, + "$(list-search) Pattern is missing. Usage: $(list-search [options] <pattern> ${list})"); + return FALSE; + } + else if (argc < 3) I have a minor improvement suggestion, it was your unit test that pointed it out: $(list-search --start-index 5 baz 5 '\"foo,\",\"bar\",\"baz\",\"bar\"')", "" I don't know how visible it is, but the *list* for searching will be the string 5 after the pattern baz, and not the long foo, bar, baz thing. The idea for improvement is to have a max cap for arguments. If argc at this point is bigger than 3, it means something can be wrong. There is no need for more positional arguments (for the use case of searching in multiple lists, I would recommend the $(list-append) templ. function). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2955?email_source=notifications&email_token=AAFOK5RHIZL34NE5PXVRUHLQXOVE5A5CNFSM4I4WYLHKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCOKWJMY#pullrequestreview-328557747>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFOK5QKM52MVSRTZGQOCKTQXOVE5ANCNFSM4I4WYLHA> .

[kira-syslogng]

Build SUCCESS