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

[Petr Cech]

Hi,

this patch set adds tests on colondb API. You can find more information 
in headers of patches.

Regards

Petr

[Lukas Slebodnik]

On (30/11/15 14:44), Petr Cech wrote:
> Hi,
>
> this patch set adds tests on colondb API. You can find more information in
> headers of patches.
>
> Regards
>
> Petr 
> From 334a4a807750dc4a149e17bbebe26c17dae267ca Mon Sep 17 00:00:00
> 2001
> From: Petr Cech <pcech(a)redhat.com&gt;
> Date: Tue, 24 Nov 2015 10:34:10 -0500
> Subject: [PATCH 1/4] COLONDB: Add comment on open function
>
> 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.
>
> So this patch adds simple lightening comment.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2764
> ---
>  src/tools/common/sss_colondb.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/tools/common/sss_colondb.h b/src/tools/common/sss_colondb.h
> index 6edd99cbe3b9ef5c86a48632ac3fc71e8a3e55fe..b2db21b0db4adb1422f4033063bea4895f591b5c
> 100644
> --- a/src/tools/common/sss_colondb.h
> +++ b/src/tools/common/sss_colondb.h
> @@ -59,6 +59,7 @@ struct sss_colondb_read_field {
>      union sss_colondb_read_data data;
>  };
>  
> +/** The close function is set by talloc_destructor. */ 
I would prefer to ellaborate
here if you decided to add doxygen comment.
I do not know which close function and what is closed.
I can only guess

>  struct sss_colondb *sss_colondb_open(TALLOC_CTX *mem_ctx,
>                                       enum sss_colondb_mode mode,
>                                       const char *filename);
> -- 
> 2.4.3
>  
> From d4196abbc6134a79970dd953c7821bbe671b2db7 Mon Sep 17 00:00:00
> 2001
> From: Petr Cech <pcech(a)redhat.com&gt;
> Date: Fri, 27 Nov 2015 06:39:37 -0500
> Subject: [PATCH 2/4] TEST_TOOLS_COLONDB: Add tests for sss_colondb_open
>
> There are three functions at API of colondb wrapper:
>  * sss_colondb_open()
>  * sss_colondb_readline()
>  * sss_colondb_writeline()
>
> This patch adds tests for sss_colondb_open() function.
>
> We test those cases:
>  * open nonexisting file for read
>  * open nonexisting file for write
>  * open existing file for read
>  * open existing file for write
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2764
> ---
>  Makefile.am                           |  19 +++
>  src/tests/cmocka/test_tools_colondb.c | 246 ++++++++++++++++++++++++++++++++++
>  2 files changed, 265 insertions(+)
>  create mode 100644 src/tests/cmocka/test_tools_colondb.c
>
> diff --git a/Makefile.am b/Makefile.am
> index d7a6f29520980d933cf0afddf8d041b2d6f3b2ef..ceafd1bf0afd856859e835717a59dceac688f147
> 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 \
> @@ -2556,6 +2557,24 @@ 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) \
> +    $(POPT_LIBS) \
> +    $(SSSD_LIBS) \ 
Do you really need all libraries from SSSD_LIBS?
    $(TALLOC_LIBS) \
    $(TEVENT_LIBS) \
    $(POPT_LIBS) \
    $(LDB_LIBS) \
    $(DBUS_LIBS) \
    $(PCRE_LIBS) \
    $(INI_CONFIG_LIBS) \
    $(COLLECTION_LIBS) \
    $(DHASH_LIBS) \
    $(OPENLDAP_LIBS) \
    $(TDB_LIBS)
    $(TALLOC_LIBS) \

> +    $(SSSD_INTERNAL_LTLIBS) \
> +    libsss_debug.la \ 
libsss_debug.la is already part of SSSD_INTERNAL_LTLIBS.
    
> +    $(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..a3f66119e7ab887138a1d03811dfd15f19b89b12
> --- /dev/null
> +++ b/src/tests/cmocka/test_tools_colondb.c
> @@ -0,0 +1,246 @@
> +/*
> +    Authors:
> +        Petr Čech <pcech(a)redhat.com&gt;
> +
> +    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"
> +
> +#define new_test_with_file(test)                                               \
> +    cmocka_unit_test_setup_teardown(test_##test, test_colondb_with_file_setup, \
> +                                    test_colondb_teardown)
> + 
I do not like macro generated code; especially after debugging nss-pam-ldapd
Moreover It's not clear from code which setup and teardwn functions are
used. Test should be crystal clear and not complicate understandind.
I know that we have such usage in other tests. But it does not mean
that we need to add new one. Moreover we wil just safe only few lines of code.

> +#define new_test_without_file(test)                                  
>          \
> +    cmocka_unit_test_setup_teardown(                                           \
> +        test_##test, test_colondb_without_file_setup, test_colondb_teardown)
> +
> +struct test_colondb_ctx
> +{
> +    bool is_file_created;
> +};
> +
> +static int _create_dir(const char *path) 
             ^
          could you explain why there is a prefix "_"
We already can distinguish between test and helper function.
Test has prefix "test_" and is also static. Should it be "_test_" ?

> +{
> +    errno_t ret;
> +
> +    ret = mkdir(path, 0775);
> +    if (ret != 0 && errno != EEXIST) {
> +        fprintf(stderr, "Could not create test directory\n");
> +        return 1; 
Please use assertions here. instead of return 1
it's not ordinary code So it does not make a sense to secial case
helper functions and test. We can use assertions on all places.
The same applies to _create_file

> +    }
> +
> +    return 0;
> +}
> +
> +static int _create_file(const char *path, const char *name)
> +{
> +    TALLOC_CTX *tmp_ctx = NULL;
> +    char *file_name = NULL;
> +    FILE *fp = NULL;
> +    errno_t ret;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (tmp_ctx == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> +        return 1;
> +    }
> +
> +    ret = _create_dir(path);
> +    if (ret != 0) {
> +        ret = 1;
> +        goto done;
> +    }
> +
> +    file_name = talloc_asprintf(tmp_ctx, "%s/%s", path, name);
> +    if (file_name == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not construct colondb path\n");
> +        ret = 1;
> +        goto done;
> +    }
> +
> +    fp = fopen(file_name, "w");
> +    if (fp == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not create file %s\n", file_name);
> +        ret = 1;
> +        goto done;
> +    }
> +    fclose(fp);
> +
> +done:
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}
> +
> +static int test_colondb_setup(void **state, bool create_file)
> +{
> +    struct test_colondb_ctx *test_ctx = NULL;
> +
> +    assert_true(leak_check_setup());
> +
> +    test_ctx = talloc_zero(global_talloc_context, struct test_colondb_ctx);
> +    assert_non_null(test_ctx);
> +
> +    if (create_file) {
> +        _create_file(TESTS_PATH, TESTS_FILE);
> +        test_ctx->is_file_created = true;
> +    } else {
> +        test_ctx->is_file_created = false;
> +    }
> +
> +    *state = test_ctx;
> +
> +    return 0;
> +}
> +
> +static int test_colondb_without_file_setup(void **state)
> +{ 
On most places we do not add prefix to setup and teardown functions.
The name of function is shorter. And IMHO only test function
should be prefxed with "_test"

We can discuss cmocka "coding style" on different thread
if you want. Just start one.

> +    return test_colondb_setup(state, false);
> +}
> +
> +static int test_colondb_with_file_setup(void **state)
> +{
> +    return test_colondb_setup(state, true);
> +}
> +
> +static int test_colondb_teardown(void **state)
> +{
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    errno_t ret;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    if (test_ctx->is_file_created) {
> +        errno = 0;
> +        ret = unlink(TESTS_PATH "/" TESTS_FILE);
> +        if (ret != 0 && errno != ENOENT) {
> +            ret = errno;
> +            DEBUG(SSSDBG_CRIT_FAILURE, "Could not delete the test config "
> +                                       "ldb file [%d]: (%s)\n",
> +                  ret, sss_strerror(ret)); 
          I would use assertions here as
well.
          Because if it fails then other test might be affected by this
          failure. It shoudl not be ignored. and debug message needn't be
          enough in some cases.
> +        }
> +    }
> +
> +    talloc_zfree(*state);
> +    test_dom_suite_cleanup(TESTS_PATH, NULL, NULL);
> +    assert_true(leak_check_teardown());
> +
> +    return 0;
> +}
> +
> +void test_open_nonexist_for_read(void **state)
> +{
> +
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    check_leaks_push(test_ctx);
> +    db =
> +        sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/"
> TESTS_FILE);
> +    assert_true(check_leaks_pop(test_ctx) == true); 
                               
            ^^^^^^^
                                        This part is redundant 
                                        also on other places
> +    assert_null(db);
> +}
> +
> +void test_open_nonexist_for_write(void **state)
> +{
> +
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    check_leaks_push(test_ctx);
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    test_ctx->is_file_created = true;
> +    assert_true(check_leaks_pop(test_ctx) == true);
> +    assert_null(db);
> +}
> +
> +void test_open_exist_for_read(void **state)
> +{
> +
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db =
> +        sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/"
> TESTS_FILE);
> +    assert_non_null(db);
> +}
> +
> +void test_open_exist_for_write(void **state)
> +{
> +
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    assert_non_null(db);
> +}
> +
> +int main(int argc, const char *argv[])
> +{
> +    poptContext pc;
> +    int opt;
> +    struct poptOption long_options[] = {
> +        POPT_AUTOHELP SSSD_DEBUG_OPTS POPT_TABLEEND};
> +
> +    const struct CMUnitTest tests[] = {
> +        new_test_without_file(open_nonexist_for_read),
> +        new_test_without_file(open_nonexist_for_write),
> +        new_test_with_file(open_exist_for_read),
> +        new_test_with_file(open_exist_for_write),
> +    };
> +
> +    /* Set debug level to invalid value so we can deside if -d 0 was used. */
> +    debug_level = SSSDBG_INVALID;
> +
> +    pc = poptGetContext(argv[0], argc, argv, long_options, 0);
> +    while ((opt = poptGetNextOpt(pc)) != -1) {
> +        switch (opt) {
> +        default:
> +            fprintf(stderr, "\nInvalid option %s: %s\n\n", poptBadOption(pc,
> 0),
> +                    poptStrerror(opt));
> +            poptPrintUsage(pc, stderr, 0);
> +            return 1;
> +        }
> +    }
> +    poptFreeContext(pc);
> +
> +    DEBUG_CLI_INIT(debug_level);
> +
> +    /* Even though normally the tests should clean up after themselves
> +     * they might not after a failed run. Remove the old db to be sure */
> +    tests_set_cwd();
> +    test_dom_suite_cleanup(TESTS_PATH, NULL, NULL);
> +
> +    return cmocka_run_group_tests(tests, NULL, NULL);
> +}
> -- 
> 2.4.3
>  
> From aa78994e2499dcfea1bc5104d6a2a81087a19259 Mon Sep 17 00:00:00
> 2001
> From: Petr Cech <pcech(a)redhat.com&gt;
> Date: Fri, 27 Nov 2015 10:07:55 -0500
> Subject: [PATCH 3/4] TEST_TOOLS_COLONDB: Test for sss_colondb_writeline
>
> There are three functions at API of colondb wrapper:
>  * sss_colondb_open()
>  * sss_colondb_readline()
>  * sss_colondb_writeline()
> This patch adds tests for sss_colondb_writeline() function.
>
> We test those cases:
>  * write to empty file
>  * write to file with existing records
>
> And tests on sss_colondb_open() with existing file were replaced
> with tests in cases of  existig empty file and existing file
> with records.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2764
> ---
>  src/tests/cmocka/test_tools_colondb.c | 158 ++++++++++++++++++++++++++++++----
>  1 file changed, 143 insertions(+), 15 deletions(-)
>
> diff --git a/src/tests/cmocka/test_tools_colondb.c
> b/src/tests/cmocka/test_tools_colondb.c
> index a3f66119e7ab887138a1d03811dfd15f19b89b12..51690a3e7bb365f8a224f385d5e114b8a42419e2
> 100644
> --- a/src/tests/cmocka/test_tools_colondb.c
> +++ b/src/tests/cmocka/test_tools_colondb.c
> @@ -28,14 +28,26 @@
>  #define TESTS_PATH "tp_" BASE_FILE_STEM
>  #define TESTS_FILE "test_colondb.ldb"
>  
> -#define new_test_with_file(test)                                               \
> -    cmocka_unit_test_setup_teardown(test_##test, test_colondb_with_file_setup, \
> -                                    test_colondb_teardown) 
Please do not add and
remove code in separate patches.
last 3 test should resolve ticket #2764.
IMHO one patch is enough.
Separate patches make sense if you do complicated changes (@see pavel's sudo
patches.) But if you only add new code It can be in single patch.

> +#define TEST_STRING1 "white"
> +#define TEST_INT1 12
> +
> +#define TEST_STRING2 "black"
> +#define TEST_INT2 34 
static constants are more type safe.

>  #define new_test_without_file(test)                                  
>          \
>      cmocka_unit_test_setup_teardown(                                           \
>          test_##test, test_colondb_without_file_setup, test_colondb_teardown)
>  
> +#define new_test_with_empty_file(test)                                         \
> +    cmocka_unit_test_setup_teardown(test_##test,                               \
> +                                    test_colondb_with_empty_file_setup,        \
> +                                    test_colondb_teardown)
> +
> +#define new_test_with_nonempty_file(test)                                      \
> +    cmocka_unit_test_setup_teardown(test_##test,                               \
> +                                    test_colondb_with_nonempty_file_setup,     \
> +                                    test_colondb_teardown)
> +
>  struct test_colondb_ctx
>  {
>      bool is_file_created;
> @@ -54,7 +66,7 @@ static int _create_dir(const char *path)
>      return 0;
>  }
>  
> -static int _create_file(const char *path, const char *name)
> +static int _create_empty_file(const char *path, const char *name)
>  { 
Please do not add and remove code in separate patches.
You can do in "wor in progress" patches.
But final version should not do a magic with renamig and moving
code here and there.


>      TALLOC_CTX *tmp_ctx = NULL;
>      char *file_name = NULL;
> @@ -93,7 +105,39 @@ done:
>      return ret;
>  }
>  
> -static int test_colondb_setup(void **state, bool create_file)
> +static int _create_nonempty_file(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}}};
> +    errno_t ret;
> +
> +    tmp_ctx = talloc_new(NULL);
> +    if (tmp_ctx == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> +        return 1;
> +    }
> +
> +    ret = _create_empty_file(TESTS_PATH, TESTS_FILE);
> +    if (ret != 0) {
> +        ret = 1;
> +        goto done;
> +    }
> +
> +    db =
> +        sss_colondb_open(tmp_ctx, SSS_COLONDB_WRITE, TESTS_PATH "/"
> TESTS_FILE);
> +
> +    ret = sss_colondb_writeline(db, table);
> +
> +done:
> +    talloc_free(tmp_ctx);
> +    return ret;
> +}
> +
> +static int test_colondb_setup(void **state, int file_state)
>  {
>      struct test_colondb_ctx *test_ctx = NULL;
>  
> @@ -102,11 +146,21 @@ static int test_colondb_setup(void **state, bool create_file)
>      test_ctx = talloc_zero(global_talloc_context, struct test_colondb_ctx);
>      assert_non_null(test_ctx);
>  
> -    if (create_file) {
> -        _create_file(TESTS_PATH, TESTS_FILE);
> +    switch (file_state) {
> +    case 0:
> +        test_ctx->is_file_created = false;
> +        break;
> +    case 1:
> +        _create_empty_file(TESTS_PATH, TESTS_FILE);
> +        test_ctx->is_file_created = true;
> +        break;
> +    case 2:
> +        _create_nonempty_file(TESTS_PATH, TESTS_FILE);
>          test_ctx->is_file_created = true;
> -    } else {
> +        break;
> +    default:
>          test_ctx->is_file_created = false;
> +        break;
>      }
>  
>      *state = test_ctx;
> @@ -116,12 +170,17 @@ static int test_colondb_setup(void **state, bool create_file)
>  
>  static int test_colondb_without_file_setup(void **state)
>  {
> -    return test_colondb_setup(state, false);
> +    return test_colondb_setup(state, 0);
>  }
>  
> -static int test_colondb_with_file_setup(void **state)
> +static int test_colondb_with_empty_file_setup(void **state)
>  {
> -    return test_colondb_setup(state, true);
> +    return test_colondb_setup(state, 1);
> +}
> +
> +static int test_colondb_with_nonempty_file_setup(void **state)
> +{
> +    return test_colondb_setup(state, 2);
>  }
>  
>  static int test_colondb_teardown(void **state)
> @@ -151,7 +210,6 @@ static int test_colondb_teardown(void **state)
>  
>  void test_open_nonexist_for_read(void **state)
>  {
> -
>      struct test_colondb_ctx *test_ctx = NULL;
>      struct sss_colondb *db = NULL;
>  
> @@ -166,7 +224,6 @@ void test_open_nonexist_for_read(void **state)
>  
>  void test_open_nonexist_for_write(void **state)
>  {
> -
>      struct test_colondb_ctx *test_ctx = NULL;
>      struct sss_colondb *db = NULL;
>  
> @@ -206,6 +263,72 @@ void test_open_exist_for_write(void **state)
>      assert_non_null(db);
>  }
>  
> +void test_open_nonempty_for_read(void **state)
> +{
> +
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db =
> +        sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/"
> TESTS_FILE);
> +    assert_non_null(db);
> +}
> +
> +void test_open_nonempty_for_write(void **state)
> +{
> +
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    assert_non_null(db);
> +}
> +
> +void test_write_to_empty(void **state)
> +{
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +    struct sss_colondb_write_field table[] = {
> +        {SSS_COLONDB_STRING, {.str = TEST_STRING1}},
> +        {SSS_COLONDB_UINT32, {.uint32 = TEST_INT1}},
> +        {SSS_COLONDB_SENTINEL, {0}}};
> +    errno_t ret;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    assert_non_null(db);
> +
> +    ret = sss_colondb_writeline(db, table);
> +    assert_true(ret == 0);
> +}
> +
> +void test_write_to_nonempty(void **state)
> +{
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +    struct sss_colondb_write_field table[] = {
> +        {SSS_COLONDB_STRING, {.str = TEST_STRING1}},
> +        {SSS_COLONDB_UINT32, {.uint32 = TEST_INT1}},
> +        {SSS_COLONDB_SENTINEL, {0}}};
> +    errno_t ret;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    assert_non_null(db);
> +
> +    ret = sss_colondb_writeline(db, table);
> +    assert_true(ret == 0);
> +}
> +
>  int main(int argc, const char *argv[])
>  {
>      poptContext pc;
> @@ -216,8 +339,13 @@ int main(int argc, const char *argv[])
>      const struct CMUnitTest tests[] = {
>          new_test_without_file(open_nonexist_for_read),
>          new_test_without_file(open_nonexist_for_write),
> -        new_test_with_file(open_exist_for_read),
> -        new_test_with_file(open_exist_for_write),
> +        new_test_with_empty_file(open_exist_for_read),
> +        new_test_with_empty_file(open_exist_for_write),
> +        new_test_with_nonempty_file(open_nonempty_for_read),
> +        new_test_with_nonempty_file(open_nonempty_for_write),
> +
> +        new_test_with_empty_file(write_to_empty),
> +        new_test_with_nonempty_file(write_to_nonempty),
>      };
>  
>      /* Set debug level to invalid value so we can deside if -d 0 was used. */
> -- 
> 2.4.3
>  
> From d3d75d8016f195459d8e4d0734f74e0b39f2d79e Mon Sep 17 00:00:00
> 2001
> From: Petr Cech <pcech(a)redhat.com&gt;
> Date: Mon, 30 Nov 2015 06:48:02 -0500
> Subject: [PATCH 4/4] TEST_TOOLS_COLONDB: Tests for sss_colondb_readline
>
> There are three functions at API of colondb wrapper:
>  * sss_colondb_open()
>  * sss_colondb_readline()
>  * sss_colondb_write_line()
> This patch adds tests for sss_colndb_readline() function.
>
> We test cases:
>  * read from file with records
>  * read from empty file
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2764
> ---
>  src/tests/cmocka/test_tools_colondb.c | 49 +++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/src/tests/cmocka/test_tools_colondb.c
> b/src/tests/cmocka/test_tools_colondb.c
> index 51690a3e7bb365f8a224f385d5e114b8a42419e2..cad699b7fcf7f22168dd87146fada67dfc512110
> 100644
> --- a/src/tests/cmocka/test_tools_colondb.c
> +++ b/src/tests/cmocka/test_tools_colondb.c
> @@ -329,6 +329,52 @@ void test_write_to_nonempty(void **state)
>      assert_true(ret == 0);
>  }
>  
> +void test_read_from_nonempty(void **state)
> +{
> +    struct test_colondb_ctx *test_ctx = 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_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db =
> +        sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/"
> TESTS_FILE);
> +    assert_non_null(db);
> +
> +    ret = sss_colondb_readline(test_ctx, db, table);
> +    assert_true(ret == 0);
> +    assert_string_equal(string, TEST_STRING2);
> +    assert_true(number == TEST_INT2); 
      Please use assert_int_equal, so in case
of failure we will see values.
Plase alsoo use appropriate cmocka assertion function also on ther places
We shoudl not use "==" in assertions.
It's hard to debug

LS

[Petr Cech]

Hi Lukas,

thank you for review. I will send new version of tests, in one patch. 
And I will address your comments.

However I would like to shed light on why I made the changes gradually. 
API consists of three functions. I wanted every patch adding just one 
feature of the API. First, I tested the function to open a database. 
Then I tested the entry into the database, which influenced the opening. 
Finally, I tested reading. I tried to build in the spirit of building a 
mathematical theory. It helped me to think about different cases. And I 
thought it would help to reviewer too...

Well, I remake it in a single patch.

Regards

Petr

[Lukas Slebodnik]

On (07/12/15 15:51), Petr Cech wrote:
> Hi Lukas,
>
> thank you for review. I will send new version of tests, in one patch. And I
> will address your comments.
>
> However I would like to shed light on why I made the changes gradually. API
> consists of three functions. I wanted every patch adding just one feature of
> the API. First, I tested the function to open a database. Then I tested the
> entry into the database, which influenced the opening. Finally, I tested
> reading. I tried to build in the spirit of building a mathematical theory. It
> helped me to think about different cases. And I thought it would help to
> reviewer too...
>  
Yes, but it's difficult to review code is you rename and move functions.
It is inevitable in some cases. But not in this case.
The purpose of ticket is add new test cases and not to move and rename
change functions. It just only mean that function was not reusable in
the 1st patch and should be changed there and not in following patch.

> Well, I remake it in a single patch.
>  
Moreover, I am not a single person who means that splitting tests
to separate patches is best idea.

Nikolai had similar request in another review.
https://lists.fedorahosted.org/archives/list/sssd-devel%40lists.fedorahos...

If you just add code then it's not a problem to review big patch
because you can read it as long article.

LS

[Lukas Slebodnik]

On (07/12/15 15:51), Petr Cech wrote:
> Hi Lukas,
>
> thank you for review. I will send new version of tests, in one patch. And I
> will address your comments.
>
> However I would like to shed light on why I made the changes gradually. API
> consists of three functions. I wanted every patch adding just one feature of
> the API. First, I tested the function to open a database. Then I tested the
> entry into the database, which influenced the opening. Finally, I tested
> reading. I tried to build in the spirit of building a mathematical theory. It
> helped me to think about different cases. And I thought it would help to
> reviewer too...
>
> Well, I remake it in a single patch.
>  
BTW there are also some coding style issues.
Your clang-format profil need some tuning :-)
but we discussed it in another thread


Here is a diff which you can inspire
-struct test_colondb_ctx
-{
only functions has "{" on next line
+struct test_colondb_ctx {
     bool is_file_created;
 };


 
@@ -110,9 +109,10 @@ static int _create_nonempty_file(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}}};
          ^                         ^^
         missing spaces            last bracket should be on separate line
                                   for struct
+        { SSS_COLONDB_STRING, {.str = TEST_STRING2 } },
+        { SSS_COLONDB_UINT32, {.uint32 = TEST_INT2 } },
+        { SSS_COLONDB_SENTINEL, { 0 } }
+    };

    db =
        sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/"
TESTS_FILE);
   ^^^^^^
   We usualy put function arguments to following line for 80+ columns on line

BTW I'm looking forward to updated clang-format profile.

LS

[Jakub Hrozek]

On Tue, Dec 08, 2015 at 09:20:52AM +0100, Lukas Slebodnik wrote:
>  On (07/12/15 15:51), Petr Cech wrote:
>  >Hi Lukas,
>  >
>  >thank you for review. I will send new version of tests, in one patch. And I
>  >will address your comments.
>  >
>  >However I would like to shed light on why I made the changes gradually. API
>  >consists of three functions. I wanted every patch adding just one feature of
>  >the API. First, I tested the function to open a database. Then I tested the
>  >entry into the database, which influenced the opening. Finally, I tested
>  >reading. I tried to build in the spirit of building a mathematical theory. It
>  >helped me to think about different cases. And I thought it would help to
>  >reviewer too...
>  >
>  Yes, but it's difficult to review code is you rename and move functions.
>  It is inevitable in some cases. But not in this case.
>  The purpose of ticket is add new test cases and not to move and rename
>  change functions. It just only mean that function was not reusable in
>  the 1st patch and should be changed there and not in following patch.
>  
>  >Well, I remake it in a single patch.
>  >
>  Moreover, I am not a single person who means that splitting tests
>  to separate patches is best idea. 
(I haven't read these patches at all, just a generic comment)

I think even better than separating patches from functionality is
separate the hunks that move something around (rename, split, ..) in
tests from adding new test code. Then the reviwer can focus on making
sure the first patches keep the same functionality and the second batch
increases coverage.

Of course there is no hard rule..

> 
> Nikolai had similar request in another review.
>
https://lists.fedorahosted.org/archives/list/sssd-devel%40lists.fedorahos...
> 
> If you just add code then it's not a problem to review big patch
> because you can read it as long article.
> 
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Petr Cech]

On 12/07/2015 03:04 PM, Lukas Slebodnik wrote:
>  On (30/11/15 14:44), Petr Cech wrote:
> > Hi,
> >
> > this patch set adds tests on colondb API. You can find more information in
> > headers of patches.
> >
> > Regards
> >
> > Petr
>
> >From 334a4a807750dc4a149e17bbebe26c17dae267ca Mon Sep 17 00:00:00 2001
> > From: Petr Cech <pcech(a)redhat.com&gt;
> > Date: Tue, 24 Nov 2015 10:34:10 -0500
> > Subject: [PATCH 1/4] COLONDB: Add comment on open function
> >
> > 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.
> >
> > So this patch adds simple lightening comment.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2764
> > ---
> > src/tools/common/sss_colondb.h | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/src/tools/common/sss_colondb.h b/src/tools/common/sss_colondb.h
> > index
> 6edd99cbe3b9ef5c86a48632ac3fc71e8a3e55fe..b2db21b0db4adb1422f4033063bea4895f591b5c 100644
> > --- a/src/tools/common/sss_colondb.h
> > +++ b/src/tools/common/sss_colondb.h
> > @@ -59,6 +59,7 @@ struct sss_colondb_read_field {
> >      union sss_colondb_read_data data;
> > };
> >
> > +/** The close function is set by talloc_destructor. */
>  I would prefer to ellaborate here if you decided to add doxygen comment.
>  I do not know which close function and what is closed.
>  I can only guess 
Well, I added doxygen comment to all functions. Close function and

comment about SSS_COLONDB_SENTINEL are what I missed most. I hope now it 
is more clearer how to use it.

> > struct sss_colondb *sss_colondb_open(TALLOC_CTX *mem_ctx,
> >                                       enum sss_colondb_mode mode,
> >                                       const char *filename);
> > --
> > 2.4.3
> >
>
> >From d4196abbc6134a79970dd953c7821bbe671b2db7 Mon Sep 17 00:00:00 2001
> > From: Petr Cech <pcech(a)redhat.com&gt;
> > Date: Fri, 27 Nov 2015 06:39:37 -0500
> > Subject: [PATCH 2/4] TEST_TOOLS_COLONDB: Add tests for sss_colondb_open
> >
> > There are three functions at API of colondb wrapper:
> > * sss_colondb_open()
> > * sss_colondb_readline()
> > * sss_colondb_writeline()
> >
> > This patch adds tests for sss_colondb_open() function.
> >
> > We test those cases:
> > * open nonexisting file for read
> > * open nonexisting file for write
> > * open existing file for read
> > * open existing file for write
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2764
> > ---
> > Makefile.am                           |  19 +++
> > src/tests/cmocka/test_tools_colondb.c | 246 ++++++++++++++++++++++++++++++++++
> > 2 files changed, 265 insertions(+)
> > create mode 100644 src/tests/cmocka/test_tools_colondb.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index
> d7a6f29520980d933cf0afddf8d041b2d6f3b2ef..ceafd1bf0afd856859e835717a59dceac688f147 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 \
> > @@ -2556,6 +2557,24 @@ 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) \
> > +    $(POPT_LIBS) \
> > +    $(SSSD_LIBS) \
>  Do you really need all libraries from SSSD_LIBS?
>       $(TALLOC_LIBS) \
>       $(TEVENT_LIBS) \
>       $(POPT_LIBS) \
>       $(LDB_LIBS) \
>       $(DBUS_LIBS) \
>       $(PCRE_LIBS) \
>       $(INI_CONFIG_LIBS) \
>       $(COLLECTION_LIBS) \
>       $(DHASH_LIBS) \
>       $(OPENLDAP_LIBS) \
>       $(TDB_LIBS)
>       $(TALLOC_LIBS) \ 
Addressed.

> > +    $(SSSD_INTERNAL_LTLIBS) \
> > +    libsss_debug.la \
>  libsss_debug.la is already part of SSSD_INTERNAL_LTLIBS. 
+1, thank you.

> > +    $(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..a3f66119e7ab887138a1d03811dfd15f19b89b12
> > --- /dev/null
> > +++ b/src/tests/cmocka/test_tools_colondb.c
> > @@ -0,0 +1,246 @@
> > +/*
> > +    Authors:
> > +        Petr Čech <pcech(a)redhat.com&gt;
> > +
> > +    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"
> > +
> > +#define new_test_with_file(test)                                               \
> > +    cmocka_unit_test_setup_teardown(test_##test, test_colondb_with_file_setup, \
> > +                                    test_colondb_teardown)
> > +
>  I do not like macro generated code; especially after debugging nss-pam-ldapd
>  Moreover It's not clear from code which setup and teardwn functions are
>  used. Test should be crystal clear and not complicate understandind.
>  I know that we have such usage in other tests. But it does not mean
>  that we need to add new one. Moreover we wil just safe only few lines of code. 
That's the problem. I have used other tests and compiled how to write 
the tests. I am afraid that we told about it some months ago. But I 
forget it.

> > +#define new_test_without_file(test)                                            \
> > +    cmocka_unit_test_setup_teardown(                                           \
> > +        test_##test, test_colondb_without_file_setup, test_colondb_teardown)
> > +
> > +struct test_colondb_ctx
> > +{
> > +    bool is_file_created;
> > +};
> > +
> > +static int _create_dir(const char *path)
>                ^
>             could you explain why there is a prefix "_"
>  We already can distinguish between test and helper function.
>  Test has prefix "test_" and is also static. Should it be "_test_" ?
I wanted one more level for hiding. It was bad wish.

> > +{
> > +    errno_t ret;
> > +
> > +    ret = mkdir(path, 0775);
> > +    if (ret != 0 && errno != EEXIST) {
> > +        fprintf(stderr, "Could not create test directory\n");
> > +        return 1;
>  Please use assertions here. instead of return 1
>  it's not ordinary code So it does not make a sense to secial case
>  helper functions and test. We can use assertions on all places.
>  The same applies to _create_file 
Addressed.

> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int _create_file(const char *path, const char *name)
> > +{
> > +    TALLOC_CTX *tmp_ctx = NULL;
> > +    char *file_name = NULL;
> > +    FILE *fp = NULL;
> > +    errno_t ret;
> > +
> > +    tmp_ctx = talloc_new(NULL);
> > +    if (tmp_ctx == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> > +        return 1;
> > +    }
> > +
> > +    ret = _create_dir(path);
> > +    if (ret != 0) {
> > +        ret = 1;
> > +        goto done;
> > +    }
> > +
> > +    file_name = talloc_asprintf(tmp_ctx, "%s/%s", path, name);
> > +    if (file_name == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not construct colondb path\n");
> > +        ret = 1;
> > +        goto done;
> > +    }
> > +
> > +    fp = fopen(file_name, "w");
> > +    if (fp == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not create file %s\n",
> file_name);
> > +        ret = 1;
> > +        goto done;
> > +    }
> > +    fclose(fp);
> > +
> > +done:
> > +    talloc_free(tmp_ctx);
> > +    return ret;
> > +}
> > +
> > +static int test_colondb_setup(void **state, bool create_file)
> > +{
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +
> > +    assert_true(leak_check_setup());
> > +
> > +    test_ctx = talloc_zero(global_talloc_context, struct test_colondb_ctx);
> > +    assert_non_null(test_ctx);
> > +
> > +    if (create_file) {
> > +        _create_file(TESTS_PATH, TESTS_FILE);
> > +        test_ctx->is_file_created = true;
> > +    } else {
> > +        test_ctx->is_file_created = false;
> > +    }
> > +
> > +    *state = test_ctx;
> > +
> > +    return 0;
> > +}
> > +
> > +static int test_colondb_without_file_setup(void **state)
> > +{
>  On most places we do not add prefix to setup and teardown functions.
>  The name of function is shorter. And IMHO only test function
>  should be prefxed with "_test"
>  
Addressed.

>  We can discuss cmocka "coding style" on different thread
>  if you want. Just start one. 
For start it could be sufficient to say what test from our tests is 
written by the preferable way. And then we could write paragraph to SSSD 
coding style.

> > +    return test_colondb_setup(state, false);
> > +}
> > +
> > +static int test_colondb_with_file_setup(void **state)
> > +{
> > +    return test_colondb_setup(state, true);
> > +}
> > +
> > +static int test_colondb_teardown(void **state)
> > +{
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    errno_t ret;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    if (test_ctx->is_file_created) {
> > +        errno = 0;
> > +        ret = unlink(TESTS_PATH "/" TESTS_FILE);
> > +        if (ret != 0 && errno != ENOENT) {
> > +            ret = errno;
> > +            DEBUG(SSSDBG_CRIT_FAILURE, "Could not delete the test config
> "
> > +                                       "ldb file [%d]: (%s)\n",
> > +                  ret, sss_strerror(ret));
>             I would use assertions here as well.
>             Because if it fails then other test might be affected by this
>             failure. It shoudl not be ignored. and debug message needn't be
>             enough in some cases. 
Addressed.

> > +        }
> > +    }
> > +
> > +    talloc_zfree(*state);
> > +    test_dom_suite_cleanup(TESTS_PATH, NULL, NULL);
> > +    assert_true(leak_check_teardown());
> > +
> > +    return 0;
> > +}
> > +
> > +void test_open_nonexist_for_read(void **state)
> > +{
> > +
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    check_leaks_push(test_ctx);
> > +    db =
> > +        sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/"
> TESTS_FILE);
> > +    assert_true(check_leaks_pop(test_ctx) == true);
>                                               ^^^^^^^
>                                           This part is redundant
>                                           also on other places 
+2 and addressed.

> > +    assert_null(db);
> > +}
> > +
> > +void test_open_nonexist_for_write(void **state)
> > +{
> > +
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    check_leaks_push(test_ctx);
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    test_ctx->is_file_created = true;
> > +    assert_true(check_leaks_pop(test_ctx) == true);
> > +    assert_null(db);
> > +}
> > +
> > +void test_open_exist_for_read(void **state)
> > +{
> > +
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db =
> > +        sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/"
> TESTS_FILE);
> > +    assert_non_null(db);
> > +}
> > +
> > +void test_open_exist_for_write(void **state)
> > +{
> > +
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    assert_non_null(db);
> > +}
> > +
> > +int main(int argc, const char *argv[])
> > +{
> > +    poptContext pc;
> > +    int opt;
> > +    struct poptOption long_options[] = {
> > +        POPT_AUTOHELP SSSD_DEBUG_OPTS POPT_TABLEEND};
> > +
> > +    const struct CMUnitTest tests[] = {
> > +        new_test_without_file(open_nonexist_for_read),
> > +        new_test_without_file(open_nonexist_for_write),
> > +        new_test_with_file(open_exist_for_read),
> > +        new_test_with_file(open_exist_for_write),
> > +    };
> > +
> > +    /* Set debug level to invalid value so we can deside if -d 0 was used. */
> > +    debug_level = SSSDBG_INVALID;
> > +
> > +    pc = poptGetContext(argv[0], argc, argv, long_options, 0);
> > +    while ((opt = poptGetNextOpt(pc)) != -1) {
> > +        switch (opt) {
> > +        default:
> > +            fprintf(stderr, "\nInvalid option %s: %s\n\n",
> poptBadOption(pc, 0),
> > +                    poptStrerror(opt));
> > +            poptPrintUsage(pc, stderr, 0);
> > +            return 1;
> > +        }
> > +    }
> > +    poptFreeContext(pc);
> > +
> > +    DEBUG_CLI_INIT(debug_level);
> > +
> > +    /* Even though normally the tests should clean up after themselves
> > +     * they might not after a failed run. Remove the old db to be sure */
> > +    tests_set_cwd();
> > +    test_dom_suite_cleanup(TESTS_PATH, NULL, NULL);
> > +
> > +    return cmocka_run_group_tests(tests, NULL, NULL);
> > +}
> > --
> > 2.4.3
> >
>
> >From aa78994e2499dcfea1bc5104d6a2a81087a19259 Mon Sep 17 00:00:00 2001
> > From: Petr Cech <pcech(a)redhat.com&gt;
> > Date: Fri, 27 Nov 2015 10:07:55 -0500
> > Subject: [PATCH 3/4] TEST_TOOLS_COLONDB: Test for sss_colondb_writeline
> >
> > There are three functions at API of colondb wrapper:
> > * sss_colondb_open()
> > * sss_colondb_readline()
> > * sss_colondb_writeline()
> > This patch adds tests for sss_colondb_writeline() function.
> >
> > We test those cases:
> > * write to empty file
> > * write to file with existing records
> >
> > And tests on sss_colondb_open() with existing file were replaced
> > with tests in cases of  existig empty file and existing file
> > with records.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2764
> > ---
> > src/tests/cmocka/test_tools_colondb.c | 158 ++++++++++++++++++++++++++++++----
> > 1 file changed, 143 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/tests/cmocka/test_tools_colondb.c
> b/src/tests/cmocka/test_tools_colondb.c
> > index
> a3f66119e7ab887138a1d03811dfd15f19b89b12..51690a3e7bb365f8a224f385d5e114b8a42419e2 100644
> > --- a/src/tests/cmocka/test_tools_colondb.c
> > +++ b/src/tests/cmocka/test_tools_colondb.c
> > @@ -28,14 +28,26 @@
> > #define TESTS_PATH "tp_" BASE_FILE_STEM
> > #define TESTS_FILE "test_colondb.ldb"
> >
> > -#define new_test_with_file(test)                                               \
> > -    cmocka_unit_test_setup_teardown(test_##test, test_colondb_with_file_setup, \
> > -                                    test_colondb_teardown)
>  Please do not add and remove code in separate patches.
>  last 3 test should resolve ticket #2764.
>  IMHO one patch is enough.
>  Separate patches make sense if you do complicated changes (@see pavel's sudo
>  patches.) But if you only add new code It can be in single patch. 
Addressed.

> > +#define TEST_STRING1 "white"
> > +#define TEST_INT1 12
> > +
> > +#define TEST_STRING2 "black"
> > +#define TEST_INT2 34
>  static constants are more type safe.
>  
Addressed. And this point should be in something like Testing Best Practice.

> > #define new_test_without_file(test)                              
>              \
> >      cmocka_unit_test_setup_teardown(                                           \
> >          test_##test, test_colondb_without_file_setup, test_colondb_teardown)
> >
> > +#define new_test_with_empty_file(test)                                         \
> > +    cmocka_unit_test_setup_teardown(test_##test,                               \
> > +                                    test_colondb_with_empty_file_setup,        \
> > +                                    test_colondb_teardown)
> > +
> > +#define new_test_with_nonempty_file(test)                                      \
> > +    cmocka_unit_test_setup_teardown(test_##test,                               \
> > +                                    test_colondb_with_nonempty_file_setup,     \
> > +                                    test_colondb_teardown)
> > +
> > struct test_colondb_ctx
> > {
> >      bool is_file_created;
> > @@ -54,7 +66,7 @@ static int _create_dir(const char *path)
> >      return 0;
> > }
> >
> > -static int _create_file(const char *path, const char *name)
> > +static int _create_empty_file(const char *path, const char *name)
> > {
>  Please do not add and remove code in separate patches.
>  You can do in "wor in progress" patches.
>  But final version should not do a magic with renamig and moving
>  code here and there. 
+1

> >      TALLOC_CTX *tmp_ctx = NULL;
> >      char *file_name = NULL;
> > @@ -93,7 +105,39 @@ done:
> >      return ret;
> > }
> >
> > -static int test_colondb_setup(void **state, bool create_file)
> > +static int _create_nonempty_file(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}}};
> > +    errno_t ret;
> > +
> > +    tmp_ctx = talloc_new(NULL);
> > +    if (tmp_ctx == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> > +        return 1;
> > +    }
> > +
> > +    ret = _create_empty_file(TESTS_PATH, TESTS_FILE);
> > +    if (ret != 0) {
> > +        ret = 1;
> > +        goto done;
> > +    }
> > +
> > +    db =
> > +        sss_colondb_open(tmp_ctx, SSS_COLONDB_WRITE, TESTS_PATH "/"
> TESTS_FILE);
> > +
> > +    ret = sss_colondb_writeline(db, table);
> > +
> > +done:
> > +    talloc_free(tmp_ctx);
> > +    return ret;
> > +}
> > +
> > +static int test_colondb_setup(void **state, int file_state)
> > {
> >      struct test_colondb_ctx *test_ctx = NULL;
> >
> > @@ -102,11 +146,21 @@ static int test_colondb_setup(void **state, bool create_file)
> >      test_ctx = talloc_zero(global_talloc_context, struct test_colondb_ctx);
> >      assert_non_null(test_ctx);
> >
> > -    if (create_file) {
> > -        _create_file(TESTS_PATH, TESTS_FILE);
> > +    switch (file_state) {
> > +    case 0:
> > +        test_ctx->is_file_created = false;
> > +        break;
> > +    case 1:
> > +        _create_empty_file(TESTS_PATH, TESTS_FILE);
> > +        test_ctx->is_file_created = true;
> > +        break;
> > +    case 2:
> > +        _create_nonempty_file(TESTS_PATH, TESTS_FILE);
> >          test_ctx->is_file_created = true;
> > -    } else {
> > +        break;
> > +    default:
> >          test_ctx->is_file_created = false;
> > +        break;
> >      }
> >
> >      *state = test_ctx;
> > @@ -116,12 +170,17 @@ static int test_colondb_setup(void **state, bool create_file)
> >
> > static int test_colondb_without_file_setup(void **state)
> > {
> > -    return test_colondb_setup(state, false);
> > +    return test_colondb_setup(state, 0);
> > }
> >
> > -static int test_colondb_with_file_setup(void **state)
> > +static int test_colondb_with_empty_file_setup(void **state)
> > {
> > -    return test_colondb_setup(state, true);
> > +    return test_colondb_setup(state, 1);
> > +}
> > +
> > +static int test_colondb_with_nonempty_file_setup(void **state)
> > +{
> > +    return test_colondb_setup(state, 2);
> > }
> >
> > static int test_colondb_teardown(void **state)
> > @@ -151,7 +210,6 @@ static int test_colondb_teardown(void **state)
> >
> > void test_open_nonexist_for_read(void **state)
> > {
> > -
> >      struct test_colondb_ctx *test_ctx = NULL;
> >      struct sss_colondb *db = NULL;
> >
> > @@ -166,7 +224,6 @@ void test_open_nonexist_for_read(void **state)
> >
> > void test_open_nonexist_for_write(void **state)
> > {
> > -
> >      struct test_colondb_ctx *test_ctx = NULL;
> >      struct sss_colondb *db = NULL;
> >
> > @@ -206,6 +263,72 @@ void test_open_exist_for_write(void **state)
> >      assert_non_null(db);
> > }
> >
> > +void test_open_nonempty_for_read(void **state)
> > +{
> > +
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db =
> > +        sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/"
> TESTS_FILE);
> > +    assert_non_null(db);
> > +}
> > +
> > +void test_open_nonempty_for_write(void **state)
> > +{
> > +
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    assert_non_null(db);
> > +}
> > +
> > +void test_write_to_empty(void **state)
> > +{
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +    struct sss_colondb_write_field table[] = {
> > +        {SSS_COLONDB_STRING, {.str = TEST_STRING1}},
> > +        {SSS_COLONDB_UINT32, {.uint32 = TEST_INT1}},
> > +        {SSS_COLONDB_SENTINEL, {0}}};
> > +    errno_t ret;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    assert_non_null(db);
> > +
> > +    ret = sss_colondb_writeline(db, table);
> > +    assert_true(ret == 0);
> > +}
> > +
> > +void test_write_to_nonempty(void **state)
> > +{
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +    struct sss_colondb_write_field table[] = {
> > +        {SSS_COLONDB_STRING, {.str = TEST_STRING1}},
> > +        {SSS_COLONDB_UINT32, {.uint32 = TEST_INT1}},
> > +        {SSS_COLONDB_SENTINEL, {0}}};
> > +    errno_t ret;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    assert_non_null(db);
> > +
> > +    ret = sss_colondb_writeline(db, table);
> > +    assert_true(ret == 0);
> > +}
> > +
> > int main(int argc, const char *argv[])
> > {
> >      poptContext pc;
> > @@ -216,8 +339,13 @@ int main(int argc, const char *argv[])
> >      const struct CMUnitTest tests[] = {
> >          new_test_without_file(open_nonexist_for_read),
> >          new_test_without_file(open_nonexist_for_write),
> > -        new_test_with_file(open_exist_for_read),
> > -        new_test_with_file(open_exist_for_write),
> > +        new_test_with_empty_file(open_exist_for_read),
> > +        new_test_with_empty_file(open_exist_for_write),
> > +        new_test_with_nonempty_file(open_nonempty_for_read),
> > +        new_test_with_nonempty_file(open_nonempty_for_write),
> > +
> > +        new_test_with_empty_file(write_to_empty),
> > +        new_test_with_nonempty_file(write_to_nonempty),
> >      };
> >
> >      /* Set debug level to invalid value so we can deside if -d 0 was used. */
> > --
> > 2.4.3
> >
>
> >From d3d75d8016f195459d8e4d0734f74e0b39f2d79e Mon Sep 17 00:00:00 2001
> > From: Petr Cech <pcech(a)redhat.com&gt;
> > Date: Mon, 30 Nov 2015 06:48:02 -0500
> > Subject: [PATCH 4/4] TEST_TOOLS_COLONDB: Tests for sss_colondb_readline
> >
> > There are three functions at API of colondb wrapper:
> > * sss_colondb_open()
> > * sss_colondb_readline()
> > * sss_colondb_write_line()
> > This patch adds tests for sss_colndb_readline() function.
> >
> > We test cases:
> > * read from file with records
> > * read from empty file
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2764
> > ---
> > src/tests/cmocka/test_tools_colondb.c | 49 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 49 insertions(+)
> >
> > diff --git a/src/tests/cmocka/test_tools_colondb.c
> b/src/tests/cmocka/test_tools_colondb.c
> > index
> 51690a3e7bb365f8a224f385d5e114b8a42419e2..cad699b7fcf7f22168dd87146fada67dfc512110 100644
> > --- a/src/tests/cmocka/test_tools_colondb.c
> > +++ b/src/tests/cmocka/test_tools_colondb.c
> > @@ -329,6 +329,52 @@ void test_write_to_nonempty(void **state)
> >      assert_true(ret == 0);
> > }
> >
> > +void test_read_from_nonempty(void **state)
> > +{
> > +    struct test_colondb_ctx *test_ctx = 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_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db =
> > +        sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/"
> TESTS_FILE);
> > +    assert_non_null(db);
> > +
> > +    ret = sss_colondb_readline(test_ctx, db, table);
> > +    assert_true(ret == 0);
> > +    assert_string_equal(string, TEST_STRING2);
> > +    assert_true(number == TEST_INT2);
>         Please use assert_int_equal, so in case of failure we will see values.
>  Plase alsoo use appropriate cmocka assertion function also on ther places
>  We shoudl not use "==" in assertions.
>  It's hard to debug 
+2 and addressed.

>  LS 
Thank you for carefull review.

Petr

[Petr Cech]

On 12/08/2015 11:40 AM, Lukas Slebodnik wrote:
>  On (07/12/15 15:51), Petr Cech wrote:
> > Hi Lukas,
> >
> > thank you for review. I will send new version of tests, in one patch. And I
> > will address your comments.
> >
> > However I would like to shed light on why I made the changes gradually. API
> > consists of three functions. I wanted every patch adding just one feature of
> > the API. First, I tested the function to open a database. Then I tested the
> > entry into the database, which influenced the opening. Finally, I tested
> > reading. I tried to build in the spirit of building a mathematical theory. It
> > helped me to think about different cases. And I thought it would help to
> > reviewer too...
> >
> > Well, I remake it in a single patch.
> >
>  BTW there are also some coding style issues.
>  Your clang-format profil need some tuning :-)
>  but we discussed it in another thread
>
>
>  Here is a diff which you can inspire
>  -struct test_colondb_ctx
>  -{
>  only functions has "{" on next line
>  +struct test_colondb_ctx {
>        bool is_file_created;
>    };
>
>
>
>  @@ -110,9 +109,10 @@ static int _create_nonempty_file(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}}};
>             ^                         ^^
>            missing spaces            last bracket should be on separate line
>                                      for struct
>  +        { SSS_COLONDB_STRING, {.str = TEST_STRING2 } },
>  +        { SSS_COLONDB_UINT32, {.uint32 = TEST_INT2 } },
>  +        { SSS_COLONDB_SENTINEL, { 0 } }
>  +    };
>
>       db =
>           sss_colondb_open(test_ctx, SSS_COLONDB_READ, TESTS_PATH "/"
> TESTS_FILE);
>      ^^^^^^
>      We usualy put function arguments to following line for 80+ columns on line
>
>  BTW I'm looking forward to updated clang-format profile.
>
>  LS 
Hi Lukas,

I addressed those comments to patch which I sent. I would like to update 
clang-format profile, but the first comes first ... I would like to take 
a look on new updated coding style which Michal sent. And I will take 
notes about it in clang-format point of view.

Regards

Petr

[Petr Cech]

bump

[Lukas Slebodnik]

On (14/12/15 08:57), Petr Cech wrote:
> bump
>  
I will take a look later today.
I'm sorry sudo patches have higther priority.

LS

[Petr Cech]

On 12/14/2015 09:02 AM, Lukas Slebodnik wrote:
>  On (14/12/15 08:57), Petr Cech wrote:
> > bump
> >
>  I will take a look later today.
>  I'm sorry sudo patches have higther priority.
>
>  LS 
It's OK. Thank you.

Petr

[Lukas Slebodnik]

On (08/12/15 14:53), Petr Cech wrote:
> On 12/07/2015 03:04 PM, Lukas Slebodnik wrote:
> >Plase alsoo use appropriate cmocka assertion function also on ther places
> >We shoudl not use "==" in assertions.
> >It's hard to debug
>
> +2 and addressed.
>
> >
> >LS
>
> Thank you for carefull review.
>
> Petr 
> From d6ef4adf46b52c4ef359f91eaf2cf7ea53cee78a Mon Sep 17 00:00:00
> 2001
> From: Petr Cech <pcech(a)redhat.com&gt;
> 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()
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2764
> ---
>  Makefile.am                           |  16 ++
>  src/tests/cmocka/test_tools_colondb.c | 414 ++++++++++++++++++++++++++++++++++
>  2 files changed, 430 insertions(+)
>  create mode 100644 src/tests/cmocka/test_tools_colondb.c
>
> diff --git a/Makefile.am b/Makefile.am
> index d7a6f29520980d933cf0afddf8d041b2d6f3b2ef..6763197a82a38f143436ac0be536dc29f51ad8a6
> 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 \
> @@ -2556,6 +2557,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..c30085aa216d63118227fb8d8459a4a14517237d
> --- /dev/null
> +++ b/src/tests/cmocka/test_tools_colondb.c
> @@ -0,0 +1,414 @@
> +/*
> +    Authors:
> +        Petr Čech <pcech(a)redhat.com&gt;
> +
> +    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 {
> +    bool is_file_created;
> +};
> +
> +static void create_dir(const char *path)
> +{
> +    errno_t ret;
> +    bool is_dir_created = false;
> +
> +    ret = mkdir(path, 0775);
> +    is_dir_created = ! (ret != 0 && errno != EEXIST); 
I'm sorry it is
still combined condition.
Even thought you create single variable for this.
The main problem is that if it failed we do not know which
part is not true and you will do not knoe value of
either "ret" or "errno" in case of failure.

I also do not thick that we shoudl continue if directory exist.
There can be leftover from previous test which will cause failures.


> +    if (!is_dir_created) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not create test directory\n");
We will not see a debug messages by default,
because by default (SSSDBG_CRIT_FAILURE   0x0020) is not logged to console

So in case of failure in mock you will not see it.
You will see just failed following assertions
which will not be enough for analysis of failure.
I would recommend to get rid of all debug messages from test.
and all assertions should be unambiguous. With simple assertion
you will immediately will know the root of problem from failed assertion.

> +    }
> +    assert_true(is_dir_created);
> +} 
> +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);
> +    if (tmp_ctx == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> +    }
> +    assert_non_null(tmp_ctx);
> +
> +    create_dir(path);
> +
> +    file_name = talloc_asprintf(tmp_ctx, "%s/%s", path, name);
> +    if (file_name == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not construct colondb path\n");
> +    }
> +    assert_non_null(file_name);
> +
> +    fp = fopen(file_name, "w");
> +    if (fp == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not create file %s\n", file_name);
> +    }
> +    assert_non_null(fp);
> +    fclose(fp);
> +
> +    talloc_free(tmp_ctx);
> +}
> +
> +static void create_nonempty_file(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(NULL);
> +    if (tmp_ctx == NULL) {
> +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> +    }
> +    assert_non_null(tmp_ctx);
> +
> +    create_empty_file(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);
> +
> +    talloc_free(tmp_ctx);
> +}
> +
> +static int setup(void **state, int file_state)
> +{
> +    struct test_colondb_ctx *test_ctx = NULL;
> +
> +    assert_true(leak_check_setup());
> +
> +    test_ctx = talloc_zero(global_talloc_context, struct test_colondb_ctx);
> +    assert_non_null(test_ctx);
> +
> +    switch (file_state) {
> +    case 0:
> +        test_ctx->is_file_created = false;
> +        break;
> +    case 1:
> +        create_empty_file(TESTS_PATH, TESTS_FILE);
> +        test_ctx->is_file_created = true;
> +        break;
> +    case 2:
> +        create_nonempty_file(TESTS_PATH, TESTS_FILE);
> +        test_ctx->is_file_created = true;
> +        break;
> +    default:
> +        test_ctx->is_file_created = false;
> +        break;
> +    }
> +
> +    *state = test_ctx;
> +
> +    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)
> +{
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    bool is_config_deleted = false;
> +    errno_t ret;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    if (test_ctx->is_file_created) {
> +        errno = 0;
> +        ret = unlink(TESTS_PATH "/" TESTS_FILE);
> +        is_config_deleted = ! (ret != 0 && errno != ENOENT);
> +        if (!is_config_deleted) {
> +            ret = errno;
> +            DEBUG(SSSDBG_CRIT_FAILURE, "Could not delete the test config "
> +                                       "ldb file [%d]: (%s)\n",
> +                  ret, sss_strerror(ret));
> +        }
> +        assert_true(is_config_deleted);
> +    }
> +
> +    talloc_zfree(*state);
> +    test_dom_suite_cleanup(TESTS_PATH, NULL, NULL);
> +    assert_true(leak_check_teardown());
> +
> +    return 0;
> +}
> +
> +void test_open_nonexist_for_read(void **state)
> +{
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    check_leaks_push(test_ctx);
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_READ,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    assert_true(check_leaks_pop(test_ctx));
> +    assert_null(db);
> +}
> +
> +void test_open_nonexist_for_write(void **state)
> +{
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    check_leaks_push(test_ctx);
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    test_ctx->is_file_created = true; 
      if you want to clean created file.
      it might be better to use different teradown function.
      rather "sending message" in test_ctx.

> +    assert_true(check_leaks_pop(test_ctx));
> +    assert_null(db);
> +}
> +
> +void test_open_exist_for_read(void **state)
> +{
> +
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_READ,
> +                          TESTS_PATH "/" TESTS_FILE); 
Why this function
does not have a check_leaks_push?
The test test_open_nonexist_for_read has it.
But I would recommend to move check leak push
to setup and check_leaks_pop to teardown
and you will avoid code duplication in tests.

> +    assert_non_null(db);
> +}
> +
> +void test_open_exist_for_write(void **state)
> +{
> +
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    assert_non_null(db);
> +}
> +
> +void test_open_nonempty_for_read(void **state)
> +{
> +
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_READ,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    assert_non_null(db);
> +}
> +
> +void test_open_nonempty_for_write(void **state)
> +{
> +
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    assert_non_null(db);
> +}
> +
> +void test_write_to_empty(void **state)
> +{
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +    struct sss_colondb_write_field table[] = {
> +        { SSS_COLONDB_STRING, { .str = TEST_STRING1 } },
> +        { SSS_COLONDB_UINT32, { .uint32 = TEST_INT1 } },
> +        { SSS_COLONDB_SENTINEL, { 0 } }
> +    };
> +    errno_t ret;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    assert_non_null(db);
> +
> +    ret = sss_colondb_writeline(db, table);
> +    assert_int_equal(ret, 0);
> +}
> +
> +void test_write_to_nonempty(void **state)
> +{
> +    struct test_colondb_ctx *test_ctx = NULL;
> +    struct sss_colondb *db = NULL;
> +    struct sss_colondb_write_field table[] = {
> +        { SSS_COLONDB_STRING, { .str = TEST_STRING1 } },
> +        { SSS_COLONDB_UINT32, { .uint32 = TEST_INT1 } },
> +        { SSS_COLONDB_SENTINEL, { 0 } }
> +    };
> +    errno_t ret;
> +
> +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    assert_non_null(db);
> +
> +    ret = sss_colondb_writeline(db, table);
> +    assert_int_equal(ret, 0);
> +}
> +
> +void test_read_from_nonempty(void **state)
> +{
> +    struct test_colondb_ctx *test_ctx = 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_get_type_abort(*state, struct test_colondb_ctx);
> +
> +    db = sss_colondb_open(test_ctx, SSS_COLONDB_READ,
> +                          TESTS_PATH "/" TESTS_FILE);
> +    assert_non_null(db);
> +
> +    ret = sss_colondb_readline(test_ctx, db, table);
> +    assert_int_equal(ret, 0);
> +    assert_string_equal(string, TEST_STRING2);
> +    assert_int_equal(number, TEST_INT2);
> +}
> + 
It would be also good to test
SSS_COLONDB_WRITE + sss_colondb_writeline
and SSS_COLONDB_READ + sss_colondb_readline
in the same test. So we will check wether content
didn't change meantime.

LS

[Petr Cech]

On 12/16/2015 02:42 PM, Lukas Slebodnik wrote:
>  On (08/12/15 14:53), Petr Cech wrote:
> > On 12/07/2015 03:04 PM, Lukas Slebodnik wrote:
> >> Plase alsoo use appropriate cmocka assertion function also on ther places
> >> We shoudl not use "==" in assertions.
> >> It's hard to debug
> >
> > +2 and addressed.
> >
> >>
> >
> >  LS 
> >
> > Thank you for carefull review.
> >
> > Petr
>
> >From d6ef4adf46b52c4ef359f91eaf2cf7ea53cee78a Mon Sep 17 00:00:00 2001
> > From: Petr Cech <pcech(a)redhat.com&gt;
> > 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()
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2764
> > ---
> > Makefile.am                           |  16 ++
> > src/tests/cmocka/test_tools_colondb.c | 414 ++++++++++++++++++++++++++++++++++
> > 2 files changed, 430 insertions(+)
> > create mode 100644 src/tests/cmocka/test_tools_colondb.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index
> d7a6f29520980d933cf0afddf8d041b2d6f3b2ef..6763197a82a38f143436ac0be536dc29f51ad8a6 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 \
> > @@ -2556,6 +2557,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..c30085aa216d63118227fb8d8459a4a14517237d
> > --- /dev/null
> > +++ b/src/tests/cmocka/test_tools_colondb.c
> > @@ -0,0 +1,414 @@
> > +/*
> > +    Authors:
> > +        Petr Čech <pcech(a)redhat.com&gt;
> > +
> > +    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 {
> > +    bool is_file_created;
> > +};
> > +
> > +static void create_dir(const char *path)
> > +{
> > +    errno_t ret;
> > +    bool is_dir_created = false;
> > +
> > +    ret = mkdir(path, 0775);
> > +    is_dir_created = ! (ret != 0 && errno != EEXIST);
>  I'm sorry it is still combined condition.
>  Even thought you create single variable for this.
>  The main problem is that if it failed we do not know which
>  part is not true and you will do not knoe value of
>  either "ret" or "errno" in case of failure.
>
>  I also do not thick that we shoudl continue if directory exist.
>  There can be leftover from previous test which will cause failures. 
Addressed.

> > +    if (!is_dir_created) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not create test directory\n");
>  We will not see a debug messages by default,
>  because by default (SSSDBG_CRIT_FAILURE   0x0020) is not logged to console
>
>  So in case of failure in mock you will not see it.
>  You will see just failed following assertions
>  which will not be enough for analysis of failure.
>  I would recommend to get rid of all debug messages from test.
>  and all assertions should be unambiguous. With simple assertion
>  you will immediately will know the root of problem from failed assertion.
Addressed.

> > +    }
> > +    assert_true(is_dir_created);
> > +}
>
> > +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);
> > +    if (tmp_ctx == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> > +    }
> > +    assert_non_null(tmp_ctx);
> > +
> > +    create_dir(path);
> > +
> > +    file_name = talloc_asprintf(tmp_ctx, "%s/%s", path, name);
> > +    if (file_name == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not construct colondb path\n");
> > +    }
> > +    assert_non_null(file_name);
> > +
> > +    fp = fopen(file_name, "w");
> > +    if (fp == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "Could not create file %s\n",
> file_name);
> > +    }
> > +    assert_non_null(fp);
> > +    fclose(fp);
> > +
> > +    talloc_free(tmp_ctx);
> > +}
> > +
> > +static void create_nonempty_file(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(NULL);
> > +    if (tmp_ctx == NULL) {
> > +        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> > +    }
> > +    assert_non_null(tmp_ctx);
> > +
> > +    create_empty_file(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);
> > +
> > +    talloc_free(tmp_ctx);
> > +}
> > +
> > +static int setup(void **state, int file_state)
> > +{
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +
> > +    assert_true(leak_check_setup());
> > +
> > +    test_ctx = talloc_zero(global_talloc_context, struct test_colondb_ctx);
> > +    assert_non_null(test_ctx);
> > +
> > +    switch (file_state) {
> > +    case 0:
> > +        test_ctx->is_file_created = false;
> > +        break;
> > +    case 1:
> > +        create_empty_file(TESTS_PATH, TESTS_FILE);
> > +        test_ctx->is_file_created = true;
> > +        break;
> > +    case 2:
> > +        create_nonempty_file(TESTS_PATH, TESTS_FILE);
> > +        test_ctx->is_file_created = true;
> > +        break;
> > +    default:
> > +        test_ctx->is_file_created = false;
> > +        break;
> > +    }
> > +
> > +    *state = test_ctx;
> > +
> > +    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)
> > +{
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    bool is_config_deleted = false;
> > +    errno_t ret;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    if (test_ctx->is_file_created) {
> > +        errno = 0;
> > +        ret = unlink(TESTS_PATH "/" TESTS_FILE);
> > +        is_config_deleted = ! (ret != 0 && errno != ENOENT);
> > +        if (!is_config_deleted) {
> > +            ret = errno;
> > +            DEBUG(SSSDBG_CRIT_FAILURE, "Could not delete the test config
> "
> > +                                       "ldb file [%d]: (%s)\n",
> > +                  ret, sss_strerror(ret));
> > +        }
> > +        assert_true(is_config_deleted);
> > +    }
> > +
> > +    talloc_zfree(*state);
> > +    test_dom_suite_cleanup(TESTS_PATH, NULL, NULL);
> > +    assert_true(leak_check_teardown());
> > +
> > +    return 0;
> > +}
> > +
> > +void test_open_nonexist_for_read(void **state)
> > +{
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    check_leaks_push(test_ctx);
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_READ,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    assert_true(check_leaks_pop(test_ctx));
> > +    assert_null(db);
> > +}
> > +
> > +void test_open_nonexist_for_write(void **state)
> > +{
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    check_leaks_push(test_ctx);
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    test_ctx->is_file_created = true;
>         if you want to clean created file.
>         it might be better to use different teradown function.
>         rather "sending message" in test_ctx. 
Addressed.

> > +    assert_true(check_leaks_pop(test_ctx));
> > +    assert_null(db);
> > +}
> > +
> > +void test_open_exist_for_read(void **state)
> > +{
> > +
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_READ,
> > +                          TESTS_PATH "/" TESTS_FILE);
>  Why this function does not have a check_leaks_push?
>  The test test_open_nonexist_for_read has it.
>  But I would recommend to move check leak push
>  to setup and check_leaks_pop to teardown
>  and you will avoid code duplication in tests. 
Addressed.

> > +    assert_non_null(db);
> > +}
> > +
> > +void test_open_exist_for_write(void **state)
> > +{
> > +
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    assert_non_null(db);
> > +}
> > +
> > +void test_open_nonempty_for_read(void **state)
> > +{
> > +
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_READ,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    assert_non_null(db);
> > +}
> > +
> > +void test_open_nonempty_for_write(void **state)
> > +{
> > +
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    assert_non_null(db);
> > +}
> > +
> > +void test_write_to_empty(void **state)
> > +{
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +    struct sss_colondb_write_field table[] = {
> > +        { SSS_COLONDB_STRING, { .str = TEST_STRING1 } },
> > +        { SSS_COLONDB_UINT32, { .uint32 = TEST_INT1 } },
> > +        { SSS_COLONDB_SENTINEL, { 0 } }
> > +    };
> > +    errno_t ret;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    assert_non_null(db);
> > +
> > +    ret = sss_colondb_writeline(db, table);
> > +    assert_int_equal(ret, 0);
> > +}
> > +
> > +void test_write_to_nonempty(void **state)
> > +{
> > +    struct test_colondb_ctx *test_ctx = NULL;
> > +    struct sss_colondb *db = NULL;
> > +    struct sss_colondb_write_field table[] = {
> > +        { SSS_COLONDB_STRING, { .str = TEST_STRING1 } },
> > +        { SSS_COLONDB_UINT32, { .uint32 = TEST_INT1 } },
> > +        { SSS_COLONDB_SENTINEL, { 0 } }
> > +    };
> > +    errno_t ret;
> > +
> > +    test_ctx = talloc_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_WRITE,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    assert_non_null(db);
> > +
> > +    ret = sss_colondb_writeline(db, table);
> > +    assert_int_equal(ret, 0);
> > +}
> > +
> > +void test_read_from_nonempty(void **state)
> > +{
> > +    struct test_colondb_ctx *test_ctx = 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_get_type_abort(*state, struct test_colondb_ctx);
> > +
> > +    db = sss_colondb_open(test_ctx, SSS_COLONDB_READ,
> > +                          TESTS_PATH "/" TESTS_FILE);
> > +    assert_non_null(db);
> > +
> > +    ret = sss_colondb_readline(test_ctx, db, table);
> > +    assert_int_equal(ret, 0);
> > +    assert_string_equal(string, TEST_STRING2);
> > +    assert_int_equal(number, TEST_INT2);
> > +}
> > +
>
>  It would be also good to test
>  SSS_COLONDB_WRITE + sss_colondb_writeline
>  and SSS_COLONDB_READ + sss_colondb_readline
>  in the same test. So we will check wether content
>  didn't change meantime. 
New test added.

>  LS 
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

[Petr Cech]

bump

[Lukas Slebodnik]

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&gt;
> 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&gt;
> 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&gt;
> +
> +    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.

LS

[Lukas Slebodnik]

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&gt;
> >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&gt;
> >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&gt;
> >+
> >+    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

[Petr Cech]

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&gt;
> >> 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&gt;
> >> 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&gt;
> >> +
> >> +    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
-- 
Petr^4 Cech

[Petr Cech]

On 02/18/2016 01:34 PM, 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&gt;
> >>> 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&gt;
> >>> 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&gt;
> >>> +
> >>> +    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 
I forget, there is link to CI
http://sssd-ci.duckdns.org/logs/job/37/53/summary.html

-- 
Petr^4 Cech

[Lukas Slebodnik]

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&gt;
> >>>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&gt;
> >>>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&gt;
> >>>+
> >>>+    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&gt;
> 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&gt;
> 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&gt;
> 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

[Petr Cech]

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

[Lukas Slebodnik]

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&gt;
> 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&gt;
> +
> +    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.

> +}
> +
> +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.

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


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


> +
> +    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

[Petr Cech]

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&gt;
> > 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&gt;
> > +
> > +    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.

-- 
Petr^4 Cech

[Lukas Slebodnik]

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&gt;
> >>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&gt;
> >>+
> >>+    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

LS

[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<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&gt;
> >>>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&gt;
> >>>+
> >>>+    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