<p dir="ltr">Thanks for the patch. Some comments below.<br></p>
<p dir="ltr">On Dec 2, 2013 4:34 PM, "Juhász Viktor" <<a href="mailto:jviktor@balabit.hu">jviktor@balabit.hu</a>> wrote:<br>
><br>
> These tests evince some problems about clone implementation.<br>
> The clone implementations didn't increased the reference of the condition,<br>
> and it leaks the template which is used by the constructors (set, subst).<br>
><br>
> These errors are corrected in this patch.<br>
><br>
> Signed-off-by: Juhász Viktor <<a href="mailto:jviktor@balabit.hu">jviktor@balabit.hu</a>><br>
> ---<br>
> lib/rewrite/Makefile.am | 2 +<br>
> lib/rewrite/rewrite-set-tag.c | 2 +-<br>
> lib/rewrite/rewrite-set.c | 4 +-<br>
> lib/rewrite/rewrite-subst.c | 4 +-<br>
> lib/rewrite/tests/Makefile.am | 9 ++++<br>
> lib/rewrite/tests/test_clone.c | 96 ++++++++++++++++++++++++++++++++++++++++++<br>
> libtest/test_case.h | 37 ++++++++++++++++<br>
> 7 files changed, 149 insertions(+), 5 deletions(-)<br>
> create mode 100644 lib/rewrite/tests/Makefile.am<br>
> create mode 100644 lib/rewrite/tests/test_clone.c<br>
> create mode 100644 libtest/test_case.h<br>
><br>
> diff --git a/lib/rewrite/Makefile.am b/lib/rewrite/Makefile.am<br>
> index b111935..bc6547f 100644<br>
> --- a/lib/rewrite/Makefile.am<br>
> +++ b/lib/rewrite/Makefile.am<br>
> @@ -21,3 +21,5 @@ BUILT_SOURCES += \<br>
> lib/rewrite/rewrite-expr-grammar.h<br>
><br>
> EXTRA_DIST += lib/rewrite/rewrite-expr-grammar.ym<br>
> +<br>
> +include lib/rewrite/tests/Makefile.am<br>
> diff --git a/lib/rewrite/rewrite-set-tag.c b/lib/rewrite/rewrite-set-tag.c<br>
> index 45d3cbf..154973d 100644<br>
> --- a/lib/rewrite/rewrite-set-tag.c<br>
> +++ b/lib/rewrite/rewrite-set-tag.c<br>
> @@ -56,7 +56,7 @@ log_rewrite_set_tag_clone(LogPipe *s)<br>
> log_rewrite_init(&cloned->super);<br>
> cloned->super.super.clone = log_rewrite_set_tag_clone;<br>
> cloned->super.process = log_rewrite_set_tag_process;<br>
> - cloned->super.condition = self->super.condition;<br>
> + cloned->super.condition = self->super.condition ? filter_expr_ref(self->super.condition) : NULL;<br>
><br>
> cloned->tag_id = self->tag_id;<br>
> cloned->value = self->value;<br>
> diff --git a/lib/rewrite/rewrite-set.c b/lib/rewrite/rewrite-set.c<br>
> index 5b117ff..de42c8d 100644<br>
> --- a/lib/rewrite/rewrite-set.c<br>
> +++ b/lib/rewrite/rewrite-set.c<br>
> @@ -56,9 +56,9 @@ log_rewrite_set_clone(LogPipe *s)<br>
> LogRewriteSet *self = (LogRewriteSet *) s;<br>
> LogRewriteSet *cloned;<br>
><br>
> - cloned = (LogRewriteSet *) log_rewrite_set_new(log_template_ref(self->value_template));<br>
> + cloned = (LogRewriteSet *) log_rewrite_set_new(self->value_template);<br>
> cloned->super.value_handle = self->super.value_handle;<br>
> - cloned->super.condition = self->super.condition;<br>
> + cloned->super.condition = self->super.condition ? filter_expr_ref(self->super.condition) : NULL;<br>
> return &cloned->super.super;<br>
> }<br>
><br>
> diff --git a/lib/rewrite/rewrite-subst.c b/lib/rewrite/rewrite-subst.c<br>
> index f16afae..385fb82 100644<br>
> --- a/lib/rewrite/rewrite-subst.c<br>
> +++ b/lib/rewrite/rewrite-subst.c<br>
> @@ -101,10 +101,10 @@ log_rewrite_subst_clone(LogPipe *s)<br>
> LogRewriteSubst *self = (LogRewriteSubst *) s;<br>
> LogRewriteSubst *cloned;<br>
><br>
> - cloned = (LogRewriteSubst *) log_rewrite_subst_new(log_template_ref(self->replacement));<br>
> + cloned = (LogRewriteSubst *) log_rewrite_subst_new(self->replacement);<br>
> cloned->matcher = log_matcher_ref(self->matcher);<br>
> cloned->super.value_handle = self->super.value_handle;<br>
> - cloned->super.condition = self->super.condition;<br>
> + cloned->super.condition = self->super.condition ? filter_expr_ref(self->super.condition) : NULL;<br>
> return &cloned->super.super;<br>
> }<br>
><br>
> diff --git a/lib/rewrite/tests/Makefile.am b/lib/rewrite/tests/Makefile.am<br>
> new file mode 100644<br>
> index 0000000..c5f740d<br>
> --- /dev/null<br>
> +++ b/lib/rewrite/tests/Makefile.am<br>
> @@ -0,0 +1,9 @@<br>
> +lib_rewrite_tests_TESTS = lib/rewrite/tests/test_clone<br>
> +<br>
> +check_PROGRAMS += ${lib_rewrite_tests_TESTS}<br>
> +<br>
> +lib_rewrite_tests_test_clone_CFLAGS = $(TEST_CFLAGS) \<br>
> + -I${top_srcdir}/lib/rewrite/tests<br>
> +<br>
> +lib_rewrite_tests_test_clone_LDADD = $(TEST_LDADD) \<br>
> + $(PREOPEN_SYSLOGFORMAT)<br>
> diff --git a/lib/rewrite/tests/test_clone.c b/lib/rewrite/tests/test_clone.c<br>
> new file mode 100644<br>
> index 0000000..8bb6d90<br>
> --- /dev/null<br>
> +++ b/lib/rewrite/tests/test_clone.c<br>
> @@ -0,0 +1,96 @@<br>
> +#include "apphook.h"<br>
> +#include "plugin.h"<br>
> +#include "testutils.h"<br>
> +#include "lib/rewrite/rewrite-expr.h"<br>
> +#include "lib/rewrite/rewrite-set.h"<br>
> +#include "lib/rewrite/rewrite-subst.h"<br>
> +#include "lib/rewrite/rewrite-set-tag.h"<br>
> +#include "lib/filter/filter-cmp.h"<br>
> +#include "test_case.h"<br>
> +<br>
> +typedef struct _RewriteTestCase {<br>
> + TestCase super;<br>
> + LogRewrite *test_set_rewrite;<br>
> + LogRewrite *test_subst_rewrite;<br>
> + LogRewrite *test_tag_rewrite;<br>
> +} RewriteTestCase;<br>
> +<br>
> +void<br>
> +rewrite_testcase_setup(TestCase *s)<br>
> +{<br>
> + RewriteTestCase *self = (RewriteTestCase *)s;<br>
> + configuration = cfg_new(0x305);<br>
> +<br>
> + FilterExprNode *set_condition = fop_cmp_new(log_template_new(configuration, "OP1"), log_template_new(configuration, "OP2"), 10520);<br>
> + FilterExprNode *subst_condition = fop_cmp_new(log_template_new(configuration, "OP1"), log_template_new(configuration, "OP2"), 10520);<br>
> + FilterExprNode *set_tag_condition = fop_cmp_new(log_template_new(configuration, "OP1"), log_template_new(configuration, "OP2"), 10520);<br>
> +<br>
> + LogTemplate *test_template = log_template_new(configuration, "TEST");<br>
> +<br>
> + self->test_set_rewrite = log_rewrite_set_new(test_template);<br>
> + self->test_subst_rewrite = log_rewrite_subst_new(test_template);<br>
> +<br>
> + self->test_tag_rewrite = log_rewrite_set_tag_new("test_tag", FALSE);<br>
> +<br>
> + log_rewrite_set_condition(self->test_set_rewrite, set_condition);<br>
> + log_rewrite_set_condition(self->test_subst_rewrite, subst_condition);<br>
> + log_rewrite_set_condition(self->test_tag_rewrite, set_tag_condition);<br>
> +<br>
> + log_rewrite_subst_set_matcher(self->test_subst_rewrite, log_matcher_posix_re_new());<br>
> +<br>
> + log_template_unref(test_template);<br>
> +<br></p>
<p dir="ltr">Lots of duplicates here. Cant we somehow reduce that?</p>
<p dir="ltr">Loop or functions?</p>
<p dir="ltr">> +<br>
> +void<br>
> +rewrite_testcase_teardown(TestCase *s)<br>
> +{<br>
> + RewriteTestCase *self = (RewriteTestCase *)s;<br>
> + log_pipe_unref((LogPipe *)self->test_subst_rewrite);<br>
> + log_pipe_unref((LogPipe *)self->test_set_rewrite);<br>
> + log_pipe_unref((LogPipe *)self->test_tag_rewrite);</p>
<p dir="ltr">Again, loop here would make more sense.<br></p>
<p dir="ltr">> + cfg_free(configuration);<br>
> +}<br>
> +<br>
> +void<br>
> +test_clone(LogRewrite *rule)<br>
> +{<br>
> + LogPipe *cloned_rule = log_pipe_clone((LogPipe *)rule);<br>
> + assert_guint32(rule->condition->ref_cnt, 2, ASSERTION_ERROR("Bad reference number of condition"));<br>
> + log_pipe_unref(cloned_rule);<br>
> + assert_guint32(rule->condition->ref_cnt, 1, ASSERTION_ERROR("Bad reference number of condition"));<br>
> +}<br></p>
<p dir="ltr">This should probably be called an assert_something</p>
<p dir="ltr">Also it should test something else the refcount is private, testing its value is fragile.<br></p>
<p dir="ltr">> +<br>
> +void<br>
> +test_reference_on_set_condition_cloned(TestCase *s)<br>
> +{<br>
> + RewriteTestCase *self = (RewriteTestCase *) s;<br>
> + test_clone(self->test_set_rewrite);<br>
> +}<br>
> +<br>
> +void<br>
> +test_reference_on_subst_condition_cloned(TestCase *s)<br>
> +{<br>
> + RewriteTestCase *self = (RewriteTestCase *) s;<br>
> + test_clone(self->test_subst_rewrite);<br>
> +}<br>
> +<br>
> +void<br>
> +test_reference_on_tag_condition_cloned(TestCase *s)<br>
> +{<br>
> + RewriteTestCase *self = (RewriteTestCase *) s;<br>
> + test_clone(self->test_tag_rewrite);<br>
> +}<br>
> +<br>
> +int<br>
> +main(int argc, char **argv)<br>
> +{<br>
> + app_startup();<br>
> + putenv("TZ=MET-1METDST");<br>
> + tzset();<br>
> +<br>
> + start_grabbing_messages();<br>
> + RUN_TESTCASE(RewriteTestCase, rewrite_testcase, test_reference_on_set_condition_cloned);<br>
> + RUN_TESTCASE(RewriteTestCase, rewrite_testcase, test_reference_on_subst_condition_cloned);<br>
> + RUN_TESTCASE(RewriteTestCase, rewrite_testcase, test_reference_on_tag_condition_cloned);<br>
> + stop_grabbing_messages();<br>
> +}<br>
> diff --git a/libtest/test_case.h b/libtest/test_case.h<br>
> new file mode 100644<br>
> index 0000000..4a49c9a<br>
> --- /dev/null<br>
> +++ b/libtest/test_case.h<br>
> @@ -0,0 +1,37 @@<br>
> +/*<br>
> + * test_case.h<br>
> + *<br>
> + * Created on: Nov 27, 2013<br>
> + * Author: jviktor<br>
> + */<br>
> +<br>
> +#ifndef TEST_CASE_H_<br>
> +#define TEST_CASE_H_<br>
> +<br>
> +typedef struct _TestCase TestCase;<br>
> +<br>
> +struct _TestCase {<br>
> + void (*setup)(TestCase *self);<br>
> + void (*run)(TestCase *self);<br>
> + void (*teardown)(TestCase *self);<br>
> +};<br>
> +<br>
> +<br>
> +static void<br>
> +run_test_case(TestCase *tc)<br>
> +{<br>
> + if (tc->setup) tc->setup(tc);<br>
> + tc->run(tc);<br>
> + if (tc->teardown) tc->teardown(tc);<br>
> +}<br>
> +<br>
> +#define RUN_TESTCASE(class_name, test_case_prefix, test_function) { \<br>
> + class_name *tc = g_new0(class_name, 1); \<br>
> + tc->super.setup = test_case_prefix ## _setup; \<br>
> + tc->super.teardown = test_case_prefix ## _teardown; \<br>
> + tc->super.run = test_function;\<br>
> + run_test_case(&tc->super); \<br>
> + g_free(tc); \<br>
> +}</p>
<p dir="ltr">Im all for working on testutils.h however I dont see what this buys for us.</p>
<p dir="ltr">It's equivalent to three lines of code without allowing descendant classes, and not really reusable.</p>
<p dir="ltr">Also, a number of current unit tests should be converted to prove it helps.<br></p>
<p dir="ltr">> +<br>
> +#endif /* TEST_CASE_H_ */<br>
> --<br>
> 1.8.3.2<br>
><br>
> ______________________________________________________________________________<br>
> Member info: <a href="https://lists.balabit.hu/mailman/listinfo/syslog-ng">https://lists.balabit.hu/mailman/listinfo/syslog-ng</a><br>
> Documentation: <a href="http://www.balabit.com/support/documentation/?product=syslog-ng">http://www.balabit.com/support/documentation/?product=syslog-ng</a><br>
> FAQ: <a href="http://www.balabit.com/wiki/syslog-ng-faq">http://www.balabit.com/wiki/syslog-ng-faq</a><br>
><br>
</p>