Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

patterndb: Add support for nested quotted string in @QSTRING@ #4717

Merged
merged 1 commit into from Nov 24, 2023

Conversation

smortex
Copy link
Contributor

@smortex smortex commented Nov 22, 2023

When using 2 chars to match quoted strings (e.g. (), [], {}`), we
want to detect nesting to capture the whole containing expression.

Matching (foo (bar (baz) qux)) quux against @QSTRING::()@ previously
captured (foo (bar (baz) and now capture (foo (bar (baz) qux)).

Fixes: #4716

Signed-off-by: Romain Tartière romain@blogreen.org

@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

1 similar comment
@kira-syslogng
Copy link
Contributor

This user does not have permission to start the build. Can one of the admins verify this patch and start the build?
(admin: you have the next options (make sure you checked the code):
"ok to test" to accept this pull request (and further changes) for testing
"test this please" for a one time test run
"add to whitelist" add author of a Pull Request to whitelist (globally, be careful, it means this user can trigger kira for any PR)
do nothing -> CI won't start)

@alltilla
Copy link
Collaborator

@kira-syslogng test this please

@smortex
Copy link
Contributor Author

smortex commented Nov 22, 2023

/__w/syslog-ng/syslog-ng/modules/correlation/radix.c:783:18: error: unused variable ‘state’ [-Werror=unused-variable]

Ooops, fixed!


if ((end = strchr(str + 1, ((gchar *)&state)[0])) != NULL)
while (*end)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while this works I'd try to retain the simplified case where you don't have separate starting/ending quotes.

strchr() is probably much faster than the loop implemented here.

e.g. I'd create two functions, one for the simple case, one for the nested case and decide which one to call in a wrapper that checks param[0] and param[1]

Copy link
Collaborator

@bazsi bazsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this a lot, thanks for contributing it and I don't think that this needs a separate parser. Previously QSTRING did not support quote pairs, and you just added it as a new feature, which also happens to support nesting.

The syntax we implemented in db-parser() is not the best one, but this change retains conventions and is not breaking compatibility.

When using 2 chars to match quoted strings (e.g. `(), `[]`, `{}`), we
want to detect nesting to capture the whole containing expression.

Matching `(foo (bar (baz) qux)) quux` against `@QSTRING::()@` previously
captured `(foo (bar (baz)` and now capture `(foo (bar (baz) qux))`.

Fixes: syslog-ng#4716

Signed-off-by: Romain Tartière <romain@blogreen.org>
@smortex
Copy link
Contributor Author

smortex commented Nov 23, 2023

Previously QSTRING did not support quote pairs

I think it did, I use it 😁

I split the code in 2 distinct functions as suggested, makes it a bit more readable.

Copy link
Collaborator

@bazsi bazsi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Thanks a lot.

@bazsi
Copy link
Collaborator

bazsi commented Nov 24, 2023

Before merging, could you please add a NEWS file snippet? I am not sure if this can still get into 4.5.0 which is being prepared right now: #4719

@bazsi bazsi mentioned this pull request Nov 24, 2023
@MrAnno
Copy link
Collaborator

MrAnno commented Nov 24, 2023

Let's include this one in v4.5.0. We'll add the entry manually.

@MrAnno MrAnno merged commit ee6d196 into syslog-ng:master Nov 24, 2023
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for nested expressions in PatternDB @QSTRING@
5 participants