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 <pc...@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 <pc...@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 <pc...@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. LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
