xml parser #1659

furiel · 2017-09-05T11:49:17Z

The xml parser can process input in xml format, and adds the parsed data to the message object.

Configuration example

parser xml_parser {
       xml(template("$MSG")
           prefix(".xml")
           drop_invalid(no)
           exclude-tags("tag1", "tag2", "tag3?", "tag*")
           strip-whitespaces(no)
          );
};

Output example

From the following xml:

<tags attr='attrval'>part1<tag1>Tag1 Leaf</tag1>part2<tag2>Tag2 Leaf</tag2>part3</tags>

the following output is generated: (through json-parse template, ignoring exclude-tags now)

{"_xml":{"tags":{"tag2":"Tag2 Leaf","tag1":"Tag1 Leaf","_attr":"attrval","tags":"part1part2part3"}}}

kira-syslogng · 2017-09-05T12:02:10Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

kira-syslogng · 2017-09-05T13:27:15Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

Kokan · 2017-09-05T14:29:49Z

modules/xml/xml.c

+  g_list_free_full(self->exclude_tags, g_free);
+  self->exclude_tags = g_list_copy(exclude_tags);
+
+  /* g_list_copy_deep would be better, but not supported on all glib versions :( */


Would it be possible to implement g_list_copy_deep under a compiler switch (do we have g_list_copy_deep or not) ?
In this case the code would document this comment :)

Thanks for the comment! I backported a g_list_copy_deep implementation to use on machines where it is not available. It is less efficient than the one provided by glib master, because this version iterates twice through the list instead of one. But it does not use any internal api from the glib list implementation (like memory allocation), so I felt safer to go with this version.

kira-syslogng · 2017-09-07T07:16:30Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

kira-syslogng · 2017-09-07T13:55:12Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

kira-syslogng · 2017-09-12T11:47:27Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

Kokan · 2017-09-12T14:55:28Z

modules/xml/xml.c

+  while (pattern != NULL)
+    {
+      gchar *str = ((gchar *)pattern->data);
+      if (strchr(str, '*') || strchr(str, '?'))


I think you could use strpbrk instead of the two strchr.

Kokan · 2017-09-13T04:11:31Z

modules/xml/CMakeLists.txt

+    ${CMAKE_CURRENT_BINARY_DIR}/xml-grammar.c
+COMPILE_FLAGS ${BISON_FLAGS})
+
+option(ENABLE_xml "Enable xml ON")


Could we use for the name of the option variable upper case ?

We decided as this is non-critical issue and general in other modules, this will not be scope of this patch.

Kokan · 2017-09-13T04:15:10Z

modules/xml/CMakeLists.txt

+    ${CMAKE_CURRENT_BINARY_DIR}/xml-grammar.c
+COMPILE_FLAGS ${BISON_FLAGS})
+
+option(ENABLE_xml "Enable xml ON")


The description is confusing, because it says "Enable xml ON", but the default value for option is OFF. It is not stated here otherwise. I would follow the pattern:

option(ENABLE_XML, "Enable xml module", ON)

We decided as this is non-critical issue and general in other modules, this will not be scope of this patch.

Kokan · 2017-09-13T04:20:14Z

modules/xml/CMakeLists.txt

+
+option(ENABLE_xml "Enable xml ON")
+
+if (ENABLE_xml)


Personally I would exclude the whole file with the condition, not just the compiling and linking part.

option(ENABLE_BLA) if (ENABLE_BLA) everything here endif()

The advantage of this that people are used to search include guards there.

We decided as this is non-critical issue and general in other modules, this will not be scope of this patch.

Kokan · 2017-09-13T05:40:10Z

modules/xml/CMakeLists.txt

+    "${CMAKE_CURRENT_BINARY_DIR}/xml-grammar.h"
+)
+
+set(xml_SOURCES


You are missing the xml.c here.

Kokan · 2017-09-13T06:42:32Z

modules/xml/tests/test_xml_parser.c

+    {"<tag1>part1<tag2>value2</tag2>part2</tag1>", ".xml.tag1", "part1part2"},
+    {"<tag1><tag11></tag11><tag12><tag121>value</tag121></tag12></tag1>", ".xml.tag1.tag12.tag121", "value"},
+    {"<tag1><tag11></tag11><tag12><tag121 attr1='1' attr2='2'>value</tag121></tag12></tag1>", ".xml.tag1.tag12.tag121._attr1", "1"},
+    {"<tag1><tag11></tag11><tag12><tag121 attr1='1' attr2='2'>value</tag121></tag12></tag1>", ".xml.tag1.tag12.tag121._attr2", "2"},


I was thinking about the following case:

<tag> <tag1>value11</tag1> <tag2>value22</tag1> </tag>

What should be the result of tag.tag1 ?

@Kokan: this is an invalid XML as tag2 is terminated by a non-matching tag.
But the question is still valid: what if an error occurs here? Do we recognize this as an error and drop the whole xml or we keep the result parsed from the valid part?

There is a drop_invalid() with default value: no. User can control this behaviour.

For the first part: if we write tag1 instead of tag2 as well:

<tag> <tag1>value11</tag1> <tag1>value22</tag1> </tag>

the result will be concatenated. The same behaviour is visible with xmllint:
$ xmllint --xpath "/tag/tag1/text()" text.xml
value11value22%
$
It is currently a limitation of the parser users cannot access the individual tags in such vector-like structures. This limitation will be documented in the administrator's guide.

furiel · 2017-09-13T07:33:38Z

modules/xml/xml-grammar.ym

+
+xml_opt
+        : KW_PREFIX '(' string ')'      { xml_parser_set_prefix(last_parser, $3); free($3); }
+        | KW_DROP_INVALID '(' yesno ')' { xml_parser_set_forward_invalid(((XMLParser *) last_parser), !$3); };


Signature must use LogParser instead of XMLParser to reduce dependency from grammar towards module.

Kokan · 2017-09-13T07:57:38Z

lib/compat/Makefile.am

@@ -17,6 +17,7 @@ compat_sources			= 	\
 	lib/compat/inet_aton.c		\
 	lib/compat/memrchr.c		\
 	lib/compat/pio.c		\
+	lib/compat/glib.c		\


You should add glib.c also to the lib/compat/CMakeLists.txt.

furiel · 2017-09-13T08:14:29Z

modules/xml/xml.c

+  InserterState *state = (InserterState *)user_data;
+  const gchar *current_value = log_msg_get_value_by_name(state->msg, state->key->str, NULL);
+  GString *value = scratch_buffers_alloc();
+  g_string_printf(value, "%s%s", current_value, text);


Text is not nullterminated here.

furiel · 2017-09-13T13:17:17Z

Added a strip_whitespaces() option. As xmllint also does not strip whitespaces, the default value is false in syslog-ng as well.
When a user creates an xml by hand and adds indentation for cosmetic purposes, the (significant) enters, tabs, spaces are maintained in the nvtable by default. But such whitespaces in the result might seem awkward in some cases, so this option can strip whitespaces if the user sets it.

kira-syslogng · 2017-09-13T13:22:32Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

kira-syslogng · 2017-09-13T14:44:08Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

kira-syslogng · 2017-09-13T15:14:28Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

furiel · 2017-09-14T08:56:06Z

modules/xml/xml.h

+  LogParser super;
+  gchar *prefix;
+  gboolean forward_invalid;
+  GList *exclude_tags;


Both exclude_tags and exclude patterns are not needed. The exclude_tags set function can compile right away, there is no need to store exclude_tags.

bazsi · 2017-09-14T09:12:09Z

Please try not to use sprintf() and friends (g_string_sprintf qualifies) as that ruins performance a lot.

lbudai

Address the questions regarding to the tests.
What about the error handling (GMarkupParser)?
Mention the limitations of this xml parser (useful information to the doc team : https://developer.gnome.org/glib/stable/glib-Simple-XML-Subset-Parser.html )
should_reverse request is optional
as well the strrchr

lbudai · 2017-09-14T09:01:16Z

lib/compat/glib.c

+{
+  GList *new_list = NULL;
+
+  if (list)


I'd return immediately instead of the if statement.
This (input validations & return/throwing exception) is a common technique for reducing the McCabe index.

lbudai · 2017-09-14T09:26:08Z

modules/xml/tests/test_xml_parser.c

+    {"<tag1>value1</tag1>", ".xml.tag1", "value1"},
+    {"<tag1 attr='attr_value'>value1</tag1>", ".xml.tag1._attr", "attr_value"},
+    {"<tag1><tag2>value2</tag2></tag1>", ".xml.tag1.tag2", "value2"},
+    {"<tag1>part1<tag2>value2</tag2>part2</tag1>", ".xml.tag1", "part1part2"},


What is the difference between

<tag1>part1<tag2>value2</tag2>part2</tag1>

and

<tag1>part1part2<tag2>value2</tag2></tag1>

These are two different xml-s, but they will result in the same keys values.
This behaviour aligns with xmllint:

$ diff -s <(xmllint --xpath "/tag1/text()" <(echo "<tag1>part1<tag2>value2</tag2>part2</tag1>")) <(xmllint --xpath "/tag1/text()" <(echo "<tag1>part1part2<tag2>value2</tag2></tag1>")) Files /proc/self/fd/11 and /proc/self/fd/13 are identical

lbudai · 2017-09-14T09:27:54Z

modules/xml/tests/test_xml_parser.c

+    {"<tag1>part1<tag2>value2</tag2>part2</tag1>", ".xml.tag1", "part1part2"},
+    {"<tag1><tag11></tag11><tag12><tag121>value</tag121></tag12></tag1>", ".xml.tag1.tag12.tag121", "value"},
+    {"<tag1><tag11></tag11><tag12><tag121 attr1='1' attr2='2'>value</tag121></tag12></tag1>", ".xml.tag1.tag12.tag121._attr1", "1"},
+    {"<tag1><tag11></tag11><tag12><tag121 attr1='1' attr2='2'>value</tag121></tag12></tag1>", ".xml.tag1.tag12.tag121._attr2", "2"},


@Kokan: this is an invalid XML as tag2 is terminated by a non-matching tag.
But the question is still valid: what if an error occurs here? Do we recognize this as an error and drop the whole xml or we keep the result parsed from the valid part?

lbudai · 2017-09-14T09:31:36Z

modules/xml/tests/test_xml_parser.c

+ParameterizedTestParameters(xmlparser, valid_inputs)
+{
+  static ValidXMLTestCase test_cases[] =
+  {


I'm missing the negative cases and more complex XMLs.

I need some suggeston on the complex XMLs, let's talk irl.

On the other hand, I have a single negative test:
Test(xml_parser, test_drop_invalid)
which tests a single unbalanced tag. I can add a list with negative cases, but I am not sure on its benefit. Those would only test the glib's xml parser implementation, which we implicitely think (work hypothesis) it works. The integration is tested with test_drop_invalid.

In the end we decided to add some failure testcase.

lbudai · 2017-09-14T09:32:48Z

modules/xml/tests/test_xml_parser.c

+    {"<longtag>Text</longtag>", "lo*gtag_break", ".xml.longtag", "Text"},
+    {"<longtag>Text</longtag>", "break_long*ag", ".xml.longtag", "Text"},
+    {"<longtag>Text</longtag>", "*tag_break", ".xml.longtag", "Text"},
+  };


Would be nice to test also more complex xmls.

lbudai · 2017-09-14T09:40:48Z

modules/xml/xml.c

+{
+  .start_element = start_element_cb,
+  .end_element = end_element_cb,
+  .text = text_cb


What about error handler and passthrough?

error
g_markup_parse_context_parse and g_markup_parse_context_end_parse also reports back a gerror, which I turn into an msg_error. For the invalid xml-s I passed, I could see the error logs. So I did not override the method. Did I overlook something and should I override?

passthrough
Comments and processing instructions should not be part of the data imo so I did not override this virtual function. By the way xmllint also strips these data from text. For example

$xmllint --xpath "/tag/text()" <(echo "<tag>value<?mso-application progid="Excel.Sheet"?></tag>") value%

lbudai · 2017-09-14T09:48:54Z

modules/xml/xml.c

+  g_free(self->prefix);
+  if (prefix)
+    self->prefix = g_strdup(prefix);
+  else


NULL-checking of prefix is superfluous as g_strdup returns NULL when its input is NULL.

lbudai · 2017-09-14T09:49:44Z

modules/xml/xml.c

+{
+  XMLParser *self = (XMLParser *) s;
+  g_free(self->prefix);
+  self->prefix = 0;


Use NULL instead of 0.

lbudai · 2017-09-14T10:00:47Z

modules/xml/xml.c

+}
+
+static gboolean
+tag_matches_patterns(const gchar *element_name, const GPtrArray *patterns, gboolean should_reverse)


What if we pass the reversed_element_name also? In this case we don't have to propagate this flag to here (and this decision already made by the caller...). On the other hand in this case it is better to pass also the length of the element_name (as the caller also has to compute it because of the reverse).

Very good comment thanks! ~~Especially again, element_name is NOT null terminated, so I should not have called strlen() on it inside this function.~~ I will fix this.

Edit: it is a little bit confusing, but only the text_cb has nonterminating gchar *, the other api calls will give a nullterminated string.

lbudai · 2017-09-14T10:04:42Z

modules/xml/xml.c

+static gint
+scan_for_last_dot(GString *str)
+{
+  gint i = str->len;


Use strrchr instead. (pos = strchr(s, '.'); return (pos-s+1); IIRC

kira-syslogng · 2017-09-14T11:28:38Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

kira-syslogng · 2017-09-15T11:11:34Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

furiel · 2017-09-15T12:35:34Z

Replaced the g_string_printf functions with a series of g_string_assign, g_string_append and g_string_append_c.

furiel · 2017-09-15T12:36:29Z

This version will not support CDATA. We will document it as a limitation.

kira-syslogng · 2017-09-15T12:45:28Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

kira-syslogng · 2017-09-15T13:05:17Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

lbudai added the in progress label Sep 5, 2017

furiel force-pushed the xml_parser branch from ec3d8d6 to 2fc8d28 Compare September 5, 2017 13:14

Kokan reviewed Sep 5, 2017

View reviewed changes

presidento added the enhancement label Sep 6, 2017

furiel force-pushed the xml_parser branch from 2fc8d28 to 91e553d Compare September 7, 2017 07:03

furiel force-pushed the xml_parser branch from 91e553d to 6a47f39 Compare September 7, 2017 13:42

furiel force-pushed the xml_parser branch from 6a47f39 to 536fdb8 Compare September 12, 2017 11:32

Kokan reviewed Sep 12, 2017

View reviewed changes

Kokan reviewed Sep 13, 2017

View reviewed changes

furiel commented Sep 13, 2017

View reviewed changes

Kokan reviewed Sep 13, 2017

View reviewed changes

furiel commented Sep 13, 2017

View reviewed changes

furiel force-pushed the xml_parser branch from f84dcf9 to b7635a1 Compare September 13, 2017 14:30

furiel force-pushed the xml_parser branch from b7635a1 to 351201e Compare September 13, 2017 15:01

furiel commented Sep 14, 2017

View reviewed changes

lbudai suggested changes Sep 14, 2017

View reviewed changes

furiel force-pushed the xml_parser branch from fcdeb87 to 8bd9ed9 Compare September 15, 2017 10:58

furiel force-pushed the xml_parser branch from 8bd9ed9 to 8204c19 Compare September 15, 2017 12:32

lbudai approved these changes Sep 15, 2017

View reviewed changes

furiel and others added 9 commits September 15, 2017 14:52

glib: g_list_copy_deep to compat header

e8f5495

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

http: refactor - g_list_copy_deep for headers

c44e8b6

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

xml-parser: new parser

3ba4047

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

xml-parser: criterion tests

521a286

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

xml-parser: drop_invalid option

ad9a7eb

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

xml-parser: exclude-tags

1ac4200

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

xml-parser: string reverse optimization

17fef64

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

xml-parser: adding strip-whitespaces option

2eb1c6a

Signed-off-by: Antal Nemes <antal.nemes@balabit.com>

unit test: new case for same tag on the same level in the tree

5e630c3

furiel force-pushed the xml_parser branch from 8204c19 to 5e630c3 Compare September 15, 2017 12:52

lbudai merged commit 0ba062e into syslog-ng:master Sep 15, 2017

lbudai removed the in progress label Sep 15, 2017

furiel deleted the xml_parser branch May 8, 2018 04:57

xml parser #1659

xml parser #1659

Conversation

furiel commented Sep 5, 2017 • edited

kira-syslogng commented Sep 5, 2017

kira-syslogng commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kira-syslogng commented Sep 7, 2017

kira-syslogng commented Sep 7, 2017

kira-syslogng commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furiel commented Sep 13, 2017

kira-syslogng commented Sep 13, 2017

kira-syslogng commented Sep 13, 2017

kira-syslogng commented Sep 13, 2017

Choose a reason for hiding this comment

bazsi commented Sep 14, 2017

lbudai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furiel Sep 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kira-syslogng commented Sep 14, 2017

kira-syslogng commented Sep 15, 2017

furiel commented Sep 15, 2017

furiel commented Sep 15, 2017

kira-syslogng commented Sep 15, 2017

kira-syslogng commented Sep 15, 2017

furiel commented Sep 5, 2017 •

edited

furiel Sep 15, 2017 •

edited