On 01/29/2016 02:55 PM, Lukas Slebodnik wrote:
> 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