[SSSD] Re: [PATCH SET] TEST_TOOLS_COLONDB: Add tests for sss_colondb_* public API

[Lukas Slebodnik]

On (24/02/16 14:01), Lukas Slebodnik wrote:
> 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
> > > > > > >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
> > > > > > >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
> > > > > > >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
> > > > > 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 
> > > > 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 <[e-mail]>
> > > > 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