On (29/01/16 14:15), Lukas Slebodnik wrote:
> 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.
http://sssd-ci.duckdns.org/logs/job/36/89/summary.html
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
http://sssd-ci.duckdns.org/logs/job/36/89/rhel7/ci-build-debug/test_tools...
LS
Hello Lukas,
thank you for careful code review. I addressed all comments and I found
memory leak in original colondb tool. Patch of memory leak is attached
in patch #1 with brief but explaining description in commit message.
Regards
--
Petr^4 Cech