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 <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_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 <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
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
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.
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