On (18/02/16 13:34), Petr Cech wrote:
>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 <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.
>>>
>>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_colondb.30639.valgrind.log
>>
>>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
>From 2bce55008b3f1a070c224bbb25191408efa42fee Mon Sep 17 00:00:00 2001
>From: Petr Cech <pc...@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
>From 016afc2aff7b980ba56690b6bafbcab377cf1e80 Mon Sep 17 00:00:00 2001
>From: Petr Cech <pc...@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
>From 036ffc7e61456baa9bbb112452bbcc483e0e07c3 Mon Sep 17 00:00:00 2001
>From: Petr Cech <pc...@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.
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
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
>From fd458598cb619ec24ef0d00a47d84c790631257b Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lsleb...@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
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
