On (24/02/16 12:47), Petr Cech wrote:
>On 02/23/2016 03:28 PM, Lukas Slebodnik wrote:
>>On (23/02/16 08:28), Petr Cech wrote:
>>>On 02/19/2016 04:33 PM, Lukas Slebodnik wrote:
>>>
>>>[...]
>>>
>>>>>>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
>>>>>From 2bce55008b3f1a070c224bbb25191408efa42fee Mon Sep 17 00:00:00
2001
>>>>>>From: Petr Cech<pcech(a)redhat.com>
>>>>>>Date: Thu, 18 Feb 2016 06:33:53 -0500
>>>>>>Subject: [PATCH 1/3] COLONDB: Fix memory leak
>>>>>>
>>>>>>man 3 getline says:
>>>>>>ssize_t getline(char **lineptr, size_t *n, FILE *stream);
>>>>>>If *lineptr is set to NULL and *n is set 0 before the call, then
>>>>>>getline() will allocate a buffer for storing the line. This
buffer
>>>>>>should be freed by the user program even if getline() failed.
>>>>>>
>>>>>>This patch fix buffer freeing in case if getline() failed.
>>>>>>
>>>>Nice catch.
>>>>
>>>>ACK
>>>Thank you, Lukas.
>>>
>>>>>From 016afc2aff7b980ba56690b6bafbcab377cf1e80 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 2/3] 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
>>>>>>---
>>>>ACK
>>>Thanks.
>>>
>>>>>From 036ffc7e61456baa9bbb112452bbcc483e0e07c3 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 3/3] 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
>>>>I'm not sure that talloc leak checking is properly used in this unit
test.
>>>
>>>I have rewrote leak checking. I hope I have removed all memory leaks from my
>>>tests.
>>>
>>>>There are leaks in setup functions:
>>>>diff --git a/src/tests/cmocka/test_tools_colondb.c
b/src/tests/cmocka/test_tools_colondb.c
>>>>index 11274d9..9cbb17f 100644
>>>>--- a/src/tests/cmocka/test_tools_colondb.c
>>>>+++ b/src/tests/cmocka/test_tools_colondb.c
>>>>@@ -61,7 +61,7 @@ static void create_empty_file(void **state, const char
*path, const char *name)
>>>> assert_non_null(fp);
>>>> fclose(fp);
>>>>
>>>>- //talloc_free(tmp_ctx);
>>>>+ talloc_free(tmp_ctx);
>>>> }
>>>>
>>>> static void create_nonempty_file(void **state,
>>>>@@ -86,7 +86,7 @@ static void create_nonempty_file(void **state,
>>>>
>>>> sss_colondb_writeline(db, table);
>>>>
>>>>- //talloc_free(tmp_ctx);
>>>>+ talloc_free(tmp_ctx);
>>>> }
>>>>
>>>>
>>>>BTW we do not use c++ style comments:-)
>>>>It's already in old coding style.
>>>>https://www.freeipa.org/page/Coding_Style#Comments
>>>
>>>Of course, it was my inattention. Sorry.
>>>
>>>>and moreover there is a leak in sss_colondb_writeline.
>>>>@see attached patch.
>>>>
>>>>I didn't try to find out what's wrong with leak check.
>>>>If you you are not able to fix it yourself then
>>>>we might take a look at test together to save some time
>>>>sending/reviewing patches.
>>>>
>>>>LS
>>>>
>>>>
>>>>0001-TOOLS-Fix-minor-memory-leak-in-sss_colondb_writeline.patch
>>>>
>>>>
>>>> From fd458598cb619ec24ef0d00a47d84c790631257b Mon Sep 17 00:00:00 2001
>>>>From: Lukas Slebodnik<lslebodn(a)redhat.com>
>>>>Date: Fri, 19 Feb 2016 16:18:02 +0100
>>>>Subject: [PATCH 1/2] TOOLS: Fix minor memory leak in
sss_colondb_writeline
>>>>
>>>>The variable line was initialized to NULL.
>>>>The we created temporary context tmp_ctx.
>>>>We use talloc_asprintf_append to append string to line which is initially
>>>>NULL and therefore new context which was not connected to tmp_ctx.
>>>> man 3 talloc_string -> talloc_asprintf_append
>>>>---
>>>> src/tools/common/sss_colondb.c | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>>diff --git a/src/tools/common/sss_colondb.c
b/src/tools/common/sss_colondb.c
>>>>index
a1a52c552b949076bdedae58f275d29a176be1e6..e8aeb315c9ed0efde15553e2d741d04c5d895b1a 100644
>>>>--- a/src/tools/common/sss_colondb.c
>>>>+++ b/src/tools/common/sss_colondb.c
>>>>@@ -202,6 +202,13 @@ errno_t sss_colondb_writeline(struct sss_colondb
*db,
>>>> return ENOMEM;
>>>> }
>>>>
>>>>+ line = talloc_strdup(tmp_ctx, "");
>>>>+ if (line == NULL) {
>>>>+ DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed.\n");
>>>>+ ret = ENOMEM;
>>>>+ goto done;
>>>>+ }
>>>>+
>>>> for (i = 0; table[i].type != SSS_COLONDB_SENTINEL; i++) {
>>>> switch (table[i].type) {
>>>> case SSS_COLONDB_UINT32:
>>>>-- 2.5.0
>>>
>>>Nice catch, ACK
>>>
>>>
>>>During code review there was problem with build on debian, so I did CI tests:
>>>http://sssd-ci.duckdns.org/logs/job/37/59/summary.html
>>>
>>>
>>>There are 4 patches attached. The first of them is from Lukas. This order of
>>>patches is valid.
>>>
>>>Regards
>>>--
>>>Petr^4 Cech
>>
>>>From 1e85c2d3dec7958356ddfb3b4b8b221816e4848e 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 4/4] 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 | 17 ++
>>>src/tests/cmocka/test_tools_colondb.c | 471
++++++++++++++++++++++++++++++++++
>>>2 files changed, 488 insertions(+)
>>>create mode 100644 src/tests/cmocka/test_tools_colondb.c
>>>
>>>diff --git a/Makefile.am b/Makefile.am
>>>index
8ff3322cd4aaaf8d419b664a18c4e0e67205beb8..4e4f38a5eaf5bfa2cfafa88b9b32848e6c27131b 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 \
>>>@@ -2576,6 +2577,22 @@ 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) \
>>>+ $(POPT_LIBS) \
>>>+ 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..123ad39bd75a0206fbf152fd35a10c6113f36bfc
>>>--- /dev/null
>>>+++ b/src/tests/cmocka/test_tools_colondb.c
>>>@@ -0,0 +1,471 @@
>>>+/*
>>>+ 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;
>>>+
>>>+static void create_dir(const char *path)
>>>+{
>>>+ errno_t ret;
>>>+
>>>+ errno = 0;
>>>+ ret = mkdir(path, 0775);
>>>+ assert_int_equal(ret, 0);
>>It's better to use assert_return_code for system calls.
>>it will also print errno in cese of failure.
>Well.
>
>>>+}
>>>+
>>>+static void create_empty_file(void **state, const char *path, const char
*name)
>>>+{
>>>+ TALLOC_CTX *tmp_ctx = NULL;
>>>+ char *file_name = NULL;
>>>+ FILE *fp = NULL;
>>>+
>>>+ tmp_ctx = talloc_new(*state);
>> ^^^^^^
>> state was not initialized yet.
>> and this function was called with
>> "create_empty_file(state ...)"
>>
>> The stare is initialized at the end of "setup"
>> function. Basically it means that you allocate on NULL
>> context. So it would be better to use already created
>> test_ctx in setup function which is protected with
>> leak_check.
>Good idea. I didn't recognize it.
>
>>
>>>+ assert_non_null(tmp_ctx);
>>>+
>>>+ create_dir(path);
>>>+
>>>+ file_name = talloc_asprintf(tmp_ctx, "%s/%s", path, name);
>>>+ assert_non_null(file_name);
>>>+
>>>+ fp = fopen(file_name, "w");
>>>+ assert_non_null(fp);
>>>+ fclose(fp);
>>>+
>>>+ talloc_free(tmp_ctx);
>>>+}
>>>+
>>>+static void create_nonempty_file(void **state,
>>>+ const char *path, const char *name)
>>>+{
>>>+ TALLOC_CTX *tmp_ctx = NULL;
>>>+ struct sss_colondb *db = NULL;
>>>+ struct sss_colondb_write_field table[] = {
>>>+ { SSS_COLONDB_STRING, { .str = TEST_STRING2 } },
>>>+ { SSS_COLONDB_UINT32, { .uint32 = TEST_INT2 } },
>>>+ { SSS_COLONDB_SENTINEL, { 0 } }
>>>+ };
>>>+
>>>+ tmp_ctx = talloc_new(*state);
>>>+ assert_non_null(tmp_ctx);
>>>+
>>>+ create_empty_file(state, TESTS_PATH, TESTS_FILE);
>>>+
>>>+ db = sss_colondb_open(tmp_ctx, SSS_COLONDB_WRITE,
>>>+ TESTS_PATH "/" TESTS_FILE);
>>>+ assert_non_null(db);
>>>+
>>>+ sss_colondb_writeline(db, table);
>> missing assert for writeline.
>Nice catch.
>
>>
>>>+ talloc_free(db);
>>>+ talloc_free(tmp_ctx);
>>>+}
>>>+
>>>+static int setup(void **state, int file_state)
>>>+{
>>>+ TALLOC_CTX *test_ctx = NULL;
>>>+
>>>+ assert_true(leak_check_setup());
>>>+
>>>+ check_leaks_push(global_talloc_context);
>>>+ test_ctx = talloc_new(global_talloc_context);
>>>+ assert_non_null(test_ctx);
>>>+
>>>+ switch (file_state) {
>>>+ case 0:
>>>+ break;
>>>+ case 1:
>>>+ create_empty_file(state, TESTS_PATH, TESTS_FILE);
>>>+ break;
>>>+ case 2:
>>>+ create_nonempty_file(state, TESTS_PATH, TESTS_FILE);
>>As I mentioned earlier, It would be good to paste "test_ctx"
>>instead of state which is initialized before return.
>>
>>>+ break;
>>>+ default:
>>>+ break;
>>>+ }
>>>+
>>>+ *state = test_ctx;
>>>+
>>BTW there is lot of code duplication in tests.
>>There is check_leaks_push in the beginning
>>and at the end of test.
>>
>>If we move it to the end of setup function and to the
>>beginning of teardown functions then test will be simpler.
>>
>>the 1st check_leaks_push in setup will check for leaks in setup
>>function and 2nd check_leaks_push will check for leaks in tests.
>>
>>>+ return 0;
>>>+}
>>>+
>>>+static int without_file_setup(void **state)
>>>+{
>>>+ return setup(state, 0);
>>>+}
>>>+
>>>+static int with_empty_file_setup(void **state)
>>>+{
>>>+ return setup(state, 1);
>>>+}
>>>+
>>>+static int with_nonempty_file_setup(void **state)
>>>+{
>>>+ return setup(state, 2);
>>>+}
>>>+
>>>+static int teardown(void **state)
>>>+{
>>>+ errno_t ret;
>>>+
>>>+ errno = 0;
>>>+ ret = unlink(TESTS_PATH "/" TESTS_FILE);
>>>+ if (ret != 0) {
>>>+ assert_int_equal(errno, ENOENT);
>>>+ }
>>>+
>>>+ talloc_zfree(*state);
>>>+ test_dom_suite_cleanup(TESTS_PATH, NULL, NULL);
>>>+ assert_true(check_leaks_pop(global_talloc_context));
>>>+ assert_true(leak_check_teardown());
>>>+
>>>+ return 0;
>>>+}
>>>+
>>//snip
>>
>>>+void test_read_from_nonempty(void **state)
>>>+{
>>>+ TALLOC_CTX *test_ctx = NULL;
>>>+ TALLOC_CTX *test_ctx_child = NULL;
>>>+ struct sss_colondb *db = NULL;
>>>+ errno_t ret;
>>>+ const char *string;
>>>+ uint32_t number;
>>>+ struct sss_colondb_read_field table[] = {
>>>+ { SSS_COLONDB_STRING, { .str = &string } },
>>>+ { SSS_COLONDB_UINT32, { .uint32 = &number } },
>>>+ { SSS_COLONDB_SENTINEL, { 0 } }
>>>+ };
>>>+
>>>+ test_ctx = talloc_new(*state);
>>>+ check_leaks_push(test_ctx);
>>>+
>>>+ db = sss_colondb_open(test_ctx, SSS_COLONDB_READ,
>>>+ TESTS_PATH "/" TESTS_FILE);
>>>+ assert_non_null(db);
>>>+
>>>+ test_ctx_child = talloc_new(test_ctx);
>>test_ctx_child is not required here. You just need to
>>release "string" after readline.
>>"test_ctx_child" could hide leaks in sss_colondb_readline.
>Thanks, that was pain for me. test_ctx_child was hack.
>
>>>+ ret = sss_colondb_readline(test_ctx_child, db, table);
>>>+ assert_int_equal(ret, 0);
>>>+ assert_string_equal(string, TEST_STRING2);
>>>+ assert_int_equal(number, TEST_INT2);
>>>+
>>>+ talloc_free(test_ctx_child);
>>>+
>>>+ talloc_free(db);
>>>+ assert_true(check_leaks_pop(test_ctx));
>>>+}
>>
>>Attached diff should address all mentioned issues.
>>
>>LS
>
>Lukas, thanks for diff. I learned many thinks. New patches are attached.
>
ACK
http://sssd-ci.duckdns.org/logs/job/37/88/summary.html
master:
* b590f44c06158485357d69cc5b24d5af05f1bb95
* cf1109e30320a994187edeb438ac7cdc36f0dd2b
* 2dd75ea79a57615808754c0ce550786edbc17d69
* 6977d7c84145ac69195be58b3330861b9b8a3b72
sssd-1-13:
* b269edafff139510ee1e9c00bdbc8f27e8aea691
* fbf7d5683287fa2c7b450b8f5b0df63673f25d83
* 34ba0c53d0d966c64ea11a6269cdd0ad985f4068
* d75ac50d0c065974a7ec2330f60657ae85e487c0
LS