On (06/01/16 13:47), Petr Cech wrote:
//snip
>Lukas, thanks for careful review.
>
>There are fixed patches attached. I hope that I check memory leaks in proper
>way.
>I am little confused if we need "struct test_colondb_ctx" because it is
empty
>now.
>
>Regards
>
>--
>Petr^4 Cech
>From 435b1c44f7b7c351129847f67e7db04898711e9e Mon Sep 17 00:00:00 2001
>From: Petr Cech <pcech(a)redhat.com>
>Date: Tue, 24 Nov 2015 10:34:10 -0500
>Subject: [PATCH 1/2] COLONDB: Add comments on functions
>
>The colondb API provides three function:
>* sss_colondb_open()
>* sss_colondb_write_field()
>* sss_colondb_read_field()
>
>It is not obvious that sss_colondb_open() add destructor on talloc
>context which close the colondb during free context. And there is
>expectation that SSS_COLONDB_SENTINEL is type of last item in line.
>
>So this patch adds simple lightening comments in doxygen style.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2764
LGTM
>From ab29fc0d22862bfd873f93982ba6a6ae9120f3b1 Mon Sep 17 00:00:00 2001
>From: Petr Cech <pcech(a)redhat.com>
>Date: Fri, 27 Nov 2015 06:39:37 -0500
>Subject: [PATCH 2/2] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
>
>There are three functions at API of colondb wrapper:
> * sss_colondb_open()
> * sss_colondb_readline()
> * sss_colondb_writeline()
>
>This patch adds tests for all of them.
>
>We test those cases:
> * open nonexisting file for read
> * open nonexisting file for write
> * open existing empty file for read
> * open existing file with records for read
> * open existing empty file for write
> * open existing file with records for write
> * write to empty file
> * write to file with existing records
> * sss_colondb_open()
> * sss_colondb_readline()
> * sss_colondb_write_line()
> * write to empty file and read it
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2764
>---
> Makefile.am | 16 ++
> src/tests/cmocka/test_tools_colondb.c | 423 ++++++++++++++++++++++++++++++++++
> 2 files changed, 439 insertions(+)
> create mode 100644 src/tests/cmocka/test_tools_colondb.c
>
>diff --git a/Makefile.am b/Makefile.am
>index
a9d3f25d3775f6ac824b9f9b85dd0412417c33d3..12ac53d0e15ff1d62f36a8ed7b9898e23992927f 100644
>--- a/Makefile.am
>+++ b/Makefile.am
>@@ -241,6 +241,7 @@ if HAVE_CMOCKA
> pam-srv-tests \
> test_ipa_subdom_util \
> test_ipa_subdom_server \
>+ test_tools_colondb \
> test_krb5_wait_queue \
> test_cert_utils \
> test_ldap_id_cleanup \
>@@ -2570,6 +2571,21 @@ test_ipa_subdom_server_LDADD = \
> libdlopen_test_providers.la \
> $(NULL)
>
>+test_tools_colondb_SOURCES = \
>+ src/tests/cmocka/test_tools_colondb.c \
>+ src/tools/common/sss_colondb.c \
>+ $(NULL)
>+test_tools_colondb_CFLAGS = \
>+ $(AM_CFLAGS) \
>+ $(NULL)
>+test_tools_colondb_LDFLAGS = \
>+ $(NULL)
>+test_tools_colondb_LDADD = \
>+ $(CMOCKA_LIBS) \
>+ $(SSSD_INTERNAL_LTLIBS) \
>+ libsss_test_common.la \
>+ $(NULL)
>+
> test_krb5_wait_queue_SOURCES = \
> src/tests/cmocka/common_mock_be.c \
> src/tests/cmocka/test_krb5_wait_queue.c \
>diff --git a/src/tests/cmocka/test_tools_colondb.c
b/src/tests/cmocka/test_tools_colondb.c
>new file mode 100644
>index
0000000000000000000000000000000000000000..5b23e593add9349e701b977b134236234e6df773
>--- /dev/null
>+++ b/src/tests/cmocka/test_tools_colondb.c
>@@ -0,0 +1,423 @@
>+/*
>+ Authors:
>+ Petr Čech <pcech(a)redhat.com>
>+
>+ Copyright (C) 2015 Red Hat
>+
>+ This program is free software; you can redistribute it and/or modify
>+ it under the terms of the GNU General Public License as published by
>+ the Free Software Foundation; either version 3 of the License, or
>+ (at your option) any later version.
>+
>+ This program is distributed in the hope that it will be useful,
>+ but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>+ GNU General Public License for more details.
>+
>+ You should have received a copy of the GNU General Public License
>+ along with this program. If not, see <
http://www.gnu.org/licenses/>.
>+*/
>+
>+#include <talloc.h>
>+#include <errno.h>
>+#include <popt.h>
>+
>+#include "tests/cmocka/common_mock.h"
>+#include "src/tools/common/sss_colondb.h"
>+
>+#define TESTS_PATH "tp_" BASE_FILE_STEM
>+#define TESTS_FILE "test_colondb.ldb"
>+
>+const char *TEST_STRING1 = "white";
>+const int TEST_INT1 = 12;
>+
>+const char *TEST_STRING2 = "black";
>+const int TEST_INT2 = 34;
>+
>+struct test_colondb_ctx {
>+ ;
>+};
"test context" is usually used for passing
values from setup funtion to tests.
If you plan to reuse in future in extended test
then it might be easier to directly use TALLOC_CTX.
>+static void create_dir(const char *path)
>+{
>+ errno_t ret;
>+
>+ errno = 0;
>+ ret = mkdir(path, 0775);
>+ assert_int_equal(ret, 0);
>+}
>+
>+static void create_empty_file(const char *path, const char *name)
>+{
>+ TALLOC_CTX *tmp_ctx = NULL;
>+ char *file_name = NULL;
>+ FILE *fp = NULL;
>+
>+ tmp_ctx = talloc_new(NULL);
Please do not allocate anything on NULL context.
It will disable leak checking leak_check_setup, leak_check_teardown
Try to look into leak checking in test
src/tests/cmocka/test_child_common.c.
And it's not important whether allocation is in test
or in setup function. It's better to be consistent,
Otherwise patch LGTM.
There are few issues it the patch set.
Linking error on debian.
/usr/bin/ld: src/tests/cmocka/test_tools_colondb-test_tools_colondb.o: undefined reference
to symbol 'poptFreeContext@@LIBPOPT_0'
//lib/x86_64-linux-gnu/libpopt.so.0: error adding symbols: DSO missing from command line
just add $(POPT_LIBS) to test_tools_colondb_LDADD in Makefile.am
and also memory leak