[syslog-ng] [PATCH] rewrite_rule_clone: unit tests for clone of rewrite rules are implemented
Balazs Scheidler
bazsi77 at gmail.com
Mon Dec 2 21:22:46 CET 2013
Thanks for the patch. Some comments below.
On Dec 2, 2013 4:34 PM, "Juhász Viktor" <jviktor at 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 at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.balabit.hu/pipermail/syslog-ng/attachments/20131202/1c82c365/attachment-0001.htm
More information about the syslog-ng
mailing list