Re: [syslog-ng] [PATCH] rewrite_rule_clone: unit tests for clone of rewrite rules are implemented

2 Dec 2013

Thanks for the patch. Some comments below.

On Dec 2, 2013 4:34 PM, "Juhász Viktor" <jviktor@balabit.hu> wrote:
...
These tests evince some problems about clone implementation.
The clone implementations didn't increased the reference of the condition,
and it leaks the template which is used by the constructors (set, subst).
These errors are corrected in this patch.
Signed-off-by: Juhász Viktor <jviktor@balabit.hu>
---
 lib/rewrite/Makefile.am        |  2 +
 lib/rewrite/rewrite-set-tag.c  |  2 +-
 lib/rewrite/rewrite-set.c      |  4 +-
 lib/rewrite/rewrite-subst.c    |  4 +-
 lib/rewrite/tests/Makefile.am  |  9 ++++
 lib/rewrite/tests/test_clone.c | 96
++++++++++++++++++++++++++++++++++++++++++
...
libtest/test_case.h            | 37 ++++++++++++++++
 7 files changed, 149 insertions(+), 5 deletions(-)
 create mode 100644 lib/rewrite/tests/Makefile.am
 create mode 100644 lib/rewrite/tests/test_clone.c
 create mode 100644 libtest/test_case.h

diff --git a/lib/rewrite/Makefile.am b/lib/rewrite/Makefile.am
index b111935..bc6547f 100644
--- a/lib/rewrite/Makefile.am
+++ b/lib/rewrite/Makefile.am
@@ -21,3 +21,5 @@ BUILT_SOURCES +=                              \
        lib/rewrite/rewrite-expr-grammar.h
EXTRA_DIST += lib/rewrite/rewrite-expr-grammar.ym
+
+include lib/rewrite/tests/Makefile.am
diff --git a/lib/rewrite/rewrite-set-tag.c b/lib/rewrite/rewrite-set-tag.c
index 45d3cbf..154973d 100644
--- a/lib/rewrite/rewrite-set-tag.c
+++ b/lib/rewrite/rewrite-set-tag.c
@@ -56,7 +56,7 @@ log_rewrite_set_tag_clone(LogPipe *s)
   log_rewrite_init(&cloned->super);
   cloned->super.super.clone = log_rewrite_set_tag_clone;
   cloned->super.process = log_rewrite_set_tag_process;
-  cloned->super.condition = self->super.condition;
+  cloned->super.condition = self->super.condition ?
filter_expr_ref(self->super.condition) : NULL;
cloned->tag_id = self->tag_id;
   cloned->value = self->value;
diff --git a/lib/rewrite/rewrite-set.c b/lib/rewrite/rewrite-set.c
index 5b117ff..de42c8d 100644
--- a/lib/rewrite/rewrite-set.c
+++ b/lib/rewrite/rewrite-set.c
@@ -56,9 +56,9 @@ log_rewrite_set_clone(LogPipe *s)
   LogRewriteSet *self = (LogRewriteSet *) s;
   LogRewriteSet *cloned;
-  cloned = (LogRewriteSet *)
log_rewrite_set_new(log_template_ref(self->value_template));
+  cloned = (LogRewriteSet *) log_rewrite_set_new(self->value_template);
   cloned->super.value_handle = self->super.value_handle;
-  cloned->super.condition = self->super.condition;
+  cloned->super.condition = self->super.condition ?
filter_expr_ref(self->super.condition) : NULL;
   return &cloned->super.super;
 }
diff --git a/lib/rewrite/rewrite-subst.c b/lib/rewrite/rewrite-subst.c
index f16afae..385fb82 100644
--- a/lib/rewrite/rewrite-subst.c
+++ b/lib/rewrite/rewrite-subst.c
@@ -101,10 +101,10 @@ log_rewrite_subst_clone(LogPipe *s)
   LogRewriteSubst *self = (LogRewriteSubst *) s;
   LogRewriteSubst *cloned;
-  cloned = (LogRewriteSubst *)
log_rewrite_subst_new(log_template_ref(self->replacement));
+  cloned = (LogRewriteSubst *) log_rewrite_subst_new(self->replacement);
   cloned->matcher = log_matcher_ref(self->matcher);
   cloned->super.value_handle = self->super.value_handle;
-  cloned->super.condition = self->super.condition;
+  cloned->super.condition = self->super.condition ?
filter_expr_ref(self->super.condition) : NULL;
   return &cloned->super.super;
 }
diff --git a/lib/rewrite/tests/Makefile.am b/lib/rewrite/tests/Makefile.am
new file mode 100644
index 0000000..c5f740d
--- /dev/null
+++ b/lib/rewrite/tests/Makefile.am
@@ -0,0 +1,9 @@
+lib_rewrite_tests_TESTS = lib/rewrite/tests/test_clone
+
+check_PROGRAMS                         += ${lib_rewrite_tests_TESTS}
+
+lib_rewrite_tests_test_clone_CFLAGS  = $(TEST_CFLAGS)    \
+  -I${top_srcdir}/lib/rewrite/tests
+
+lib_rewrite_tests_test_clone_LDADD      = $(TEST_LDADD)        \
+       $(PREOPEN_SYSLOGFORMAT)
diff --git a/lib/rewrite/tests/test_clone.c
b/lib/rewrite/tests/test_clone.c
new file mode 100644
index 0000000..8bb6d90
--- /dev/null
+++ b/lib/rewrite/tests/test_clone.c
@@ -0,0 +1,96 @@
+#include "apphook.h"
+#include "plugin.h"
+#include "testutils.h"
+#include "lib/rewrite/rewrite-expr.h"
+#include "lib/rewrite/rewrite-set.h"
+#include "lib/rewrite/rewrite-subst.h"
+#include "lib/rewrite/rewrite-set-tag.h"
+#include "lib/filter/filter-cmp.h"
+#include "test_case.h"
+
+typedef struct _RewriteTestCase {
+  TestCase super;
+  LogRewrite *test_set_rewrite;
+  LogRewrite *test_subst_rewrite;
+  LogRewrite *test_tag_rewrite;
+} RewriteTestCase;
+
+void
+rewrite_testcase_setup(TestCase *s)
+{
+  RewriteTestCase *self = (RewriteTestCase *)s;
+  configuration = cfg_new(0x305);
+
+  FilterExprNode *set_condition =
fop_cmp_new(log_template_new(configuration, "OP1"),
log_template_new(configuration, "OP2"), 10520);
+  FilterExprNode *subst_condition =
fop_cmp_new(log_template_new(configuration, "OP1"),
log_template_new(configuration, "OP2"), 10520);
+  FilterExprNode *set_tag_condition =
fop_cmp_new(log_template_new(configuration, "OP1"),
log_template_new(configuration, "OP2"), 10520);
+
+  LogTemplate *test_template = log_template_new(configuration, "TEST");
+
+  self->test_set_rewrite = log_rewrite_set_new(test_template);
+  self->test_subst_rewrite = log_rewrite_subst_new(test_template);
+
+  self->test_tag_rewrite = log_rewrite_set_tag_new("test_tag", FALSE);
+
+  log_rewrite_set_condition(self->test_set_rewrite, set_condition);
+  log_rewrite_set_condition(self->test_subst_rewrite, subst_condition);
+  log_rewrite_set_condition(self->test_tag_rewrite, set_tag_condition);
+
+  log_rewrite_subst_set_matcher(self->test_subst_rewrite,
log_matcher_posix_re_new());
+
+  log_template_unref(test_template);
+
Lots of duplicates here. Cant we somehow reduce that?

Loop or functions?
...
+
+void
+rewrite_testcase_teardown(TestCase *s)
+{
+  RewriteTestCase *self = (RewriteTestCase *)s;
+  log_pipe_unref((LogPipe *)self->test_subst_rewrite);
+  log_pipe_unref((LogPipe *)self->test_set_rewrite);
+  log_pipe_unref((LogPipe *)self->test_tag_rewrite);
Again, loop here would make more sense.
...
+  cfg_free(configuration);
+}
+
+void
+test_clone(LogRewrite *rule)
+{
+  LogPipe *cloned_rule = log_pipe_clone((LogPipe *)rule);
+  assert_guint32(rule->condition->ref_cnt, 2, ASSERTION_ERROR("Bad
reference number of condition"));
+  log_pipe_unref(cloned_rule);
+  assert_guint32(rule->condition->ref_cnt, 1, ASSERTION_ERROR("Bad
reference number of condition"));
+}
This should probably be called an assert_something

Also it should test something else the refcount is private, testing its
value is fragile.
...
+
+void
+test_reference_on_set_condition_cloned(TestCase *s)
+{
+  RewriteTestCase *self = (RewriteTestCase *) s;
+  test_clone(self->test_set_rewrite);
+}
+
+void
+test_reference_on_subst_condition_cloned(TestCase *s)
+{
+  RewriteTestCase *self = (RewriteTestCase *) s;
+  test_clone(self->test_subst_rewrite);
+}
+
+void
+test_reference_on_tag_condition_cloned(TestCase *s)
+{
+  RewriteTestCase *self = (RewriteTestCase *) s;
+  test_clone(self->test_tag_rewrite);
+}
+
+int
+main(int argc, char **argv)
+{
+       app_startup();
+       putenv("TZ=MET-1METDST");
+       tzset();
+
+       start_grabbing_messages();
+       RUN_TESTCASE(RewriteTestCase, rewrite_testcase,
test_reference_on_set_condition_cloned);
+       RUN_TESTCASE(RewriteTestCase, rewrite_testcase,
test_reference_on_subst_condition_cloned);
+       RUN_TESTCASE(RewriteTestCase, rewrite_testcase,
test_reference_on_tag_condition_cloned);
+       stop_grabbing_messages();
+}
diff --git a/libtest/test_case.h b/libtest/test_case.h
new file mode 100644
index 0000000..4a49c9a
--- /dev/null
+++ b/libtest/test_case.h
@@ -0,0 +1,37 @@
+/*
+ * test_case.h
+ *
+ *  Created on: Nov 27, 2013
+ *      Author: jviktor
+ */
+
+#ifndef TEST_CASE_H_
+#define TEST_CASE_H_
+
+typedef struct _TestCase TestCase;
+
+struct _TestCase {
+  void (*setup)(TestCase *self);
+  void (*run)(TestCase *self);
+  void (*teardown)(TestCase *self);
+};
+
+
+static void
+run_test_case(TestCase *tc)
+{
+  if (tc->setup) tc->setup(tc);
+  tc->run(tc);
+  if (tc->teardown) tc->teardown(tc);
+}
+
+#define RUN_TESTCASE(class_name, test_case_prefix, test_function) { \
+  class_name *tc = g_new0(class_name, 1); \
+  tc->super.setup = test_case_prefix ## _setup; \
+  tc->super.teardown = test_case_prefix ## _teardown; \
+  tc->super.run = test_function;\
+  run_test_case(&tc->super); \
+  g_free(tc); \
+}
Im all for working on testutils.h however I dont see what this buys for us.

It's equivalent to three lines of code without allowing descendant classes,
and not really reusable.

Also, a number of current unit tests should be converted to prove it helps.
...
+
+#endif /* TEST_CASE_H_ */
--
1.8.3.2
______________________________________________________________________________
...
Member info: https://lists.balabit.hu/mailman/listinfo/syslog-ng
Documentation:
http://www.balabit.com/support/documentation/?product=syslog-ng
FAQ: http://www.balabit.com/wiki/syslog-ng-faq


    

Re: [syslog-ng] [PATCH] rewrite_rule_clone: unit tests for clone of rewrite rules are implemented

Balazs Scheidler