Hi,
this patch set adds tests on colondb API. You can find more information in headers of patches.
Regards
Petr
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@redhat.com 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@redhat.com 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@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+#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@redhat.com 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@redhat.com 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
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
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.fedorahosted...
If you just add code then it's not a problem to review big patch because you can read it as long article.
LS
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.fedorahosted...
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@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
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
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
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@redhat.com 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@redhat.com 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@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+#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@redhat.com 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@redhat.com 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
bump
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
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@redhat.com Date: Fri, 27 Nov 2015 06:39:37 -0500 Subject: [PATCH 2/2] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
There are three functions at API of colondb wrapper:
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_writeline()
This patch adds tests for all of them.
We test those cases:
- open nonexisting file for read
- open nonexisting file for write
- open existing empty file for read
- open existing file with records for read
- open existing empty file for write
- open existing file with records for write
- write to empty file
- write to file with existing records
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_write_line()
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@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+const char *TEST_STRING1 = "white"; +const int TEST_INT1 = 12;
+const char *TEST_STRING2 = "black"; +const int TEST_INT2 = 34;
+struct test_colondb_ctx {
- 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
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@redhat.com Date: Fri, 27 Nov 2015 06:39:37 -0500 Subject: [PATCH 2/2] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
There are three functions at API of colondb wrapper:
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_writeline()
This patch adds tests for all of them.
We test those cases:
- open nonexisting file for read
- open nonexisting file for write
- open existing empty file for read
- open existing file with records for read
- open existing empty file for write
- open existing file with records for write
- write to empty file
- write to file with existing records
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_write_line()
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@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+const char *TEST_STRING1 = "white"; +const int TEST_INT1 = 12;
+const char *TEST_STRING2 = "black"; +const int TEST_INT2 = 34;
+struct test_colondb_ctx {
- 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
bump
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@redhat.com Date: Tue, 24 Nov 2015 10:34:10 -0500 Subject: [PATCH 1/2] COLONDB: Add comments on functions
The colondb API provides three function:
- sss_colondb_open()
- sss_colondb_write_field()
- sss_colondb_read_field()
It is not obvious that sss_colondb_open() add destructor on talloc context which close the colondb during free context. And there is expectation that SSS_COLONDB_SENTINEL is type of last item in line.
So this patch adds simple lightening comments in doxygen style.
LGTM
From ab29fc0d22862bfd873f93982ba6a6ae9120f3b1 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@redhat.com Date: Fri, 27 Nov 2015 06:39:37 -0500 Subject: [PATCH 2/2] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
There are three functions at API of colondb wrapper:
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_writeline()
This patch adds tests for all of them.
We test those cases:
- open nonexisting file for read
- open nonexisting file for write
- open existing empty file for read
- open existing file with records for read
- open existing empty file for write
- open existing file with records for write
- write to empty file
- write to file with existing records
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_write_line()
- write to empty file and read it
Resolves: https://fedorahosted.org/sssd/ticket/2764
Makefile.am | 16 ++ src/tests/cmocka/test_tools_colondb.c | 423 ++++++++++++++++++++++++++++++++++ 2 files changed, 439 insertions(+) create mode 100644 src/tests/cmocka/test_tools_colondb.c
diff --git a/Makefile.am b/Makefile.am index a9d3f25d3775f6ac824b9f9b85dd0412417c33d3..12ac53d0e15ff1d62f36a8ed7b9898e23992927f 100644 --- a/Makefile.am +++ b/Makefile.am @@ -241,6 +241,7 @@ if HAVE_CMOCKA pam-srv-tests \ test_ipa_subdom_util \ test_ipa_subdom_server \
test_tools_colondb \ test_krb5_wait_queue \ test_cert_utils \ test_ldap_id_cleanup \@@ -2570,6 +2571,21 @@ test_ipa_subdom_server_LDADD = \ libdlopen_test_providers.la \ $(NULL)
+test_tools_colondb_SOURCES = \
- src/tests/cmocka/test_tools_colondb.c \
- src/tools/common/sss_colondb.c \
- $(NULL)
+test_tools_colondb_CFLAGS = \
- $(AM_CFLAGS) \
- $(NULL)
+test_tools_colondb_LDFLAGS = \
- $(NULL)
+test_tools_colondb_LDADD = \
- $(CMOCKA_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la \
- $(NULL)
test_krb5_wait_queue_SOURCES = \ src/tests/cmocka/common_mock_be.c \ src/tests/cmocka/test_krb5_wait_queue.c \ diff --git a/src/tests/cmocka/test_tools_colondb.c b/src/tests/cmocka/test_tools_colondb.c new file mode 100644 index 0000000000000000000000000000000000000000..5b23e593add9349e701b977b134236234e6df773 --- /dev/null +++ b/src/tests/cmocka/test_tools_colondb.c @@ -0,0 +1,423 @@ +/*
- Authors:
Petr Čech <pcech@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+const char *TEST_STRING1 = "white"; +const int TEST_INT1 = 12;
+const char *TEST_STRING2 = "black"; +const int TEST_INT2 = 34;
+struct test_colondb_ctx {
- ;
+};
"test context" is usually used for passing values from setup funtion to tests.
If you plan to reuse in future in extended test then it might be easier to directly use TALLOC_CTX.
+static void create_dir(const char *path) +{
- errno_t ret;
- errno = 0;
- ret = mkdir(path, 0775);
- assert_int_equal(ret, 0);
+}
+static void create_empty_file(const char *path, const char *name) +{
- TALLOC_CTX *tmp_ctx = NULL;
- char *file_name = NULL;
- FILE *fp = NULL;
- tmp_ctx = talloc_new(NULL);
Please do not allocate anything on NULL context. It will disable leak checking leak_check_setup, leak_check_teardown Try to look into leak checking in test src/tests/cmocka/test_child_common.c. And it's not important whether allocation is in test or in setup function. It's better to be consistent,
Otherwise patch LGTM.
LS
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@redhat.com Date: Tue, 24 Nov 2015 10:34:10 -0500 Subject: [PATCH 1/2] COLONDB: Add comments on functions
The colondb API provides three function:
- sss_colondb_open()
- sss_colondb_write_field()
- sss_colondb_read_field()
It is not obvious that sss_colondb_open() add destructor on talloc context which close the colondb during free context. And there is expectation that SSS_COLONDB_SENTINEL is type of last item in line.
So this patch adds simple lightening comments in doxygen style.
LGTM
From ab29fc0d22862bfd873f93982ba6a6ae9120f3b1 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@redhat.com Date: Fri, 27 Nov 2015 06:39:37 -0500 Subject: [PATCH 2/2] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
There are three functions at API of colondb wrapper:
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_writeline()
This patch adds tests for all of them.
We test those cases:
- open nonexisting file for read
- open nonexisting file for write
- open existing empty file for read
- open existing file with records for read
- open existing empty file for write
- open existing file with records for write
- write to empty file
- write to file with existing records
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_write_line()
- write to empty file and read it
Resolves: https://fedorahosted.org/sssd/ticket/2764
Makefile.am | 16 ++ src/tests/cmocka/test_tools_colondb.c | 423 ++++++++++++++++++++++++++++++++++ 2 files changed, 439 insertions(+) create mode 100644 src/tests/cmocka/test_tools_colondb.c
diff --git a/Makefile.am b/Makefile.am index a9d3f25d3775f6ac824b9f9b85dd0412417c33d3..12ac53d0e15ff1d62f36a8ed7b9898e23992927f 100644 --- a/Makefile.am +++ b/Makefile.am @@ -241,6 +241,7 @@ if HAVE_CMOCKA pam-srv-tests \ test_ipa_subdom_util \ test_ipa_subdom_server \
test_tools_colondb \ test_krb5_wait_queue \ test_cert_utils \ test_ldap_id_cleanup \@@ -2570,6 +2571,21 @@ test_ipa_subdom_server_LDADD = \ libdlopen_test_providers.la \ $(NULL)
+test_tools_colondb_SOURCES = \
- src/tests/cmocka/test_tools_colondb.c \
- src/tools/common/sss_colondb.c \
- $(NULL)
+test_tools_colondb_CFLAGS = \
- $(AM_CFLAGS) \
- $(NULL)
+test_tools_colondb_LDFLAGS = \
- $(NULL)
+test_tools_colondb_LDADD = \
- $(CMOCKA_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la \
- $(NULL)
test_krb5_wait_queue_SOURCES = \ src/tests/cmocka/common_mock_be.c \ src/tests/cmocka/test_krb5_wait_queue.c \ diff --git a/src/tests/cmocka/test_tools_colondb.c b/src/tests/cmocka/test_tools_colondb.c new file mode 100644 index 0000000000000000000000000000000000000000..5b23e593add9349e701b977b134236234e6df773 --- /dev/null +++ b/src/tests/cmocka/test_tools_colondb.c @@ -0,0 +1,423 @@ +/*
- Authors:
Petr Čech <pcech@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+const char *TEST_STRING1 = "white"; +const int TEST_INT1 = 12;
+const char *TEST_STRING2 = "black"; +const int TEST_INT2 = 34;
+struct test_colondb_ctx {
- ;
+};
"test context" is usually used for passing values from setup funtion to tests.
If you plan to reuse in future in extended test then it might be easier to directly use TALLOC_CTX.
+static void create_dir(const char *path) +{
- errno_t ret;
- errno = 0;
- ret = mkdir(path, 0775);
- assert_int_equal(ret, 0);
+}
+static void create_empty_file(const char *path, const char *name) +{
- TALLOC_CTX *tmp_ctx = NULL;
- char *file_name = NULL;
- FILE *fp = NULL;
- tmp_ctx = talloc_new(NULL);
Please do not allocate anything on NULL context. It will disable leak checking leak_check_setup, leak_check_teardown Try to look into leak checking in test src/tests/cmocka/test_child_common.c. And it's not important whether allocation is in test or in setup function. It's better to be consistent,Otherwise patch LGTM.
There are few issues it the patch set. http://sssd-ci.duckdns.org/logs/job/36/89/summary.html
Linking error on debian. /usr/bin/ld: src/tests/cmocka/test_tools_colondb-test_tools_colondb.o: undefined reference to symbol 'poptFreeContext@@LIBPOPT_0' //lib/x86_64-linux-gnu/libpopt.so.0: error adding symbols: DSO missing from command line
just add $(POPT_LIBS) to test_tools_colondb_LDADD in Makefile.am
and also memory leak http://sssd-ci.duckdns.org/logs/job/36/89/rhel7/ci-build-debug/test_tools_co...
LS
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@redhat.com Date: Tue, 24 Nov 2015 10:34:10 -0500 Subject: [PATCH 1/2] COLONDB: Add comments on functions
The colondb API provides three function:
- sss_colondb_open()
- sss_colondb_write_field()
- sss_colondb_read_field()
It is not obvious that sss_colondb_open() add destructor on talloc context which close the colondb during free context. And there is expectation that SSS_COLONDB_SENTINEL is type of last item in line.
So this patch adds simple lightening comments in doxygen style.
LGTM
From ab29fc0d22862bfd873f93982ba6a6ae9120f3b1 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@redhat.com Date: Fri, 27 Nov 2015 06:39:37 -0500 Subject: [PATCH 2/2] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
There are three functions at API of colondb wrapper:
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_writeline()
This patch adds tests for all of them.
We test those cases:
- open nonexisting file for read
- open nonexisting file for write
- open existing empty file for read
- open existing file with records for read
- open existing empty file for write
- open existing file with records for write
- write to empty file
- write to file with existing records
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_write_line()
- write to empty file and read it
Resolves: https://fedorahosted.org/sssd/ticket/2764
Makefile.am | 16 ++ src/tests/cmocka/test_tools_colondb.c | 423 ++++++++++++++++++++++++++++++++++ 2 files changed, 439 insertions(+) create mode 100644 src/tests/cmocka/test_tools_colondb.c
diff --git a/Makefile.am b/Makefile.am index a9d3f25d3775f6ac824b9f9b85dd0412417c33d3..12ac53d0e15ff1d62f36a8ed7b9898e23992927f 100644 --- a/Makefile.am +++ b/Makefile.am @@ -241,6 +241,7 @@ if HAVE_CMOCKA pam-srv-tests \ test_ipa_subdom_util \ test_ipa_subdom_server \
test_tools_colondb \ test_krb5_wait_queue \ test_cert_utils \ test_ldap_id_cleanup \@@ -2570,6 +2571,21 @@ test_ipa_subdom_server_LDADD = \ libdlopen_test_providers.la \ $(NULL)
+test_tools_colondb_SOURCES = \
- src/tests/cmocka/test_tools_colondb.c \
- src/tools/common/sss_colondb.c \
- $(NULL)
+test_tools_colondb_CFLAGS = \
- $(AM_CFLAGS) \
- $(NULL)
+test_tools_colondb_LDFLAGS = \
- $(NULL)
+test_tools_colondb_LDADD = \
- $(CMOCKA_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la \
- $(NULL)
test_krb5_wait_queue_SOURCES = \ src/tests/cmocka/common_mock_be.c \ src/tests/cmocka/test_krb5_wait_queue.c \ diff --git a/src/tests/cmocka/test_tools_colondb.c b/src/tests/cmocka/test_tools_colondb.c new file mode 100644 index 0000000000000000000000000000000000000000..5b23e593add9349e701b977b134236234e6df773 --- /dev/null +++ b/src/tests/cmocka/test_tools_colondb.c @@ -0,0 +1,423 @@ +/*
- Authors:
Petr Čech <pcech@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+const char *TEST_STRING1 = "white"; +const int TEST_INT1 = 12;
+const char *TEST_STRING2 = "black"; +const int TEST_INT2 = 34;
+struct test_colondb_ctx {
- ;
+};
"test context" is usually used for passing values from setup funtion to tests.
If you plan to reuse in future in extended test then it might be easier to directly use TALLOC_CTX.
+static void create_dir(const char *path) +{
- errno_t ret;
- errno = 0;
- ret = mkdir(path, 0775);
- assert_int_equal(ret, 0);
+}
+static void create_empty_file(const char *path, const char *name) +{
- TALLOC_CTX *tmp_ctx = NULL;
- char *file_name = NULL;
- FILE *fp = NULL;
- tmp_ctx = talloc_new(NULL);
Please do not allocate anything on NULL context. It will disable leak checking leak_check_setup, leak_check_teardown Try to look into leak checking in test src/tests/cmocka/test_child_common.c. And it's not important whether allocation is in test or in setup function. It's better to be consistent,Otherwise patch LGTM.
There are few issues it the patch set. http://sssd-ci.duckdns.org/logs/job/36/89/summary.html
Linking error on debian. /usr/bin/ld: src/tests/cmocka/test_tools_colondb-test_tools_colondb.o: undefined reference to symbol 'poptFreeContext@@LIBPOPT_0' //lib/x86_64-linux-gnu/libpopt.so.0: error adding symbols: DSO missing from command line
just add $(POPT_LIBS) to test_tools_colondb_LDADD in Makefile.am
and also memory leak http://sssd-ci.duckdns.org/logs/job/36/89/rhel7/ci-build-debug/test_tools_co...
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
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@redhat.com Date: Tue, 24 Nov 2015 10:34:10 -0500 Subject: [PATCH 1/2] COLONDB: Add comments on functions
The colondb API provides three function:
- sss_colondb_open()
- sss_colondb_write_field()
- sss_colondb_read_field()
It is not obvious that sss_colondb_open() add destructor on talloc context which close the colondb during free context. And there is expectation that SSS_COLONDB_SENTINEL is type of last item in line.
So this patch adds simple lightening comments in doxygen style.
LGTM
From ab29fc0d22862bfd873f93982ba6a6ae9120f3b1 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@redhat.com Date: Fri, 27 Nov 2015 06:39:37 -0500 Subject: [PATCH 2/2] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
There are three functions at API of colondb wrapper:
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_writeline()
This patch adds tests for all of them.
We test those cases:
- open nonexisting file for read
- open nonexisting file for write
- open existing empty file for read
- open existing file with records for read
- open existing empty file for write
- open existing file with records for write
- write to empty file
- write to file with existing records
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_write_line()
- write to empty file and read it
Resolves: https://fedorahosted.org/sssd/ticket/2764
Makefile.am | 16 ++ src/tests/cmocka/test_tools_colondb.c | 423 ++++++++++++++++++++++++++++++++++ 2 files changed, 439 insertions(+) create mode 100644 src/tests/cmocka/test_tools_colondb.c
diff --git a/Makefile.am b/Makefile.am index a9d3f25d3775f6ac824b9f9b85dd0412417c33d3..12ac53d0e15ff1d62f36a8ed7b9898e23992927f 100644 --- a/Makefile.am +++ b/Makefile.am @@ -241,6 +241,7 @@ if HAVE_CMOCKA pam-srv-tests \ test_ipa_subdom_util \ test_ipa_subdom_server \
test_tools_colondb \ test_krb5_wait_queue \ test_cert_utils \ test_ldap_id_cleanup \@@ -2570,6 +2571,21 @@ test_ipa_subdom_server_LDADD = \ libdlopen_test_providers.la \ $(NULL)
+test_tools_colondb_SOURCES = \
- src/tests/cmocka/test_tools_colondb.c \
- src/tools/common/sss_colondb.c \
- $(NULL)
+test_tools_colondb_CFLAGS = \
- $(AM_CFLAGS) \
- $(NULL)
+test_tools_colondb_LDFLAGS = \
- $(NULL)
+test_tools_colondb_LDADD = \
- $(CMOCKA_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la \
- $(NULL)
test_krb5_wait_queue_SOURCES = \ src/tests/cmocka/common_mock_be.c \ src/tests/cmocka/test_krb5_wait_queue.c \ diff --git a/src/tests/cmocka/test_tools_colondb.c b/src/tests/cmocka/test_tools_colondb.c new file mode 100644 index 0000000000000000000000000000000000000000..5b23e593add9349e701b977b134236234e6df773
--- /dev/null +++ b/src/tests/cmocka/test_tools_colondb.c @@ -0,0 +1,423 @@ +/*
- Authors:
Petr Čech <pcech@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or
modify
- it under the terms of the GNU General Public License as
published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see
http://www.gnu.org/licenses/. +*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+const char *TEST_STRING1 = "white"; +const int TEST_INT1 = 12;
+const char *TEST_STRING2 = "black"; +const int TEST_INT2 = 34;
+struct test_colondb_ctx {
- ;
+};
"test context" is usually used for passing values from setup funtion to tests.
If you plan to reuse in future in extended test then it might be easier to directly use TALLOC_CTX.
+static void create_dir(const char *path) +{
- errno_t ret;
- errno = 0;
- ret = mkdir(path, 0775);
- assert_int_equal(ret, 0);
+}
+static void create_empty_file(const char *path, const char *name) +{
- TALLOC_CTX *tmp_ctx = NULL;
- char *file_name = NULL;
- FILE *fp = NULL;
- tmp_ctx = talloc_new(NULL);
Please do not allocate anything on NULL context. It will disable leak checking leak_check_setup,leak_check_teardown Try to look into leak checking in test src/tests/cmocka/test_child_common.c. And it's not important whether allocation is in test or in setup function. It's better to be consistent,
Otherwise patch LGTM.
There are few issues it the patch set. http://sssd-ci.duckdns.org/logs/job/36/89/summary.html
Linking error on debian. /usr/bin/ld: src/tests/cmocka/test_tools_colondb-test_tools_colondb.o: undefined reference to symbol 'poptFreeContext@@LIBPOPT_0' //lib/x86_64-linux-gnu/libpopt.so.0: error adding symbols: DSO missing from command line
just add $(POPT_LIBS) to test_tools_colondb_LDADD in Makefile.am
and also memory leak http://sssd-ci.duckdns.org/logs/job/36/89/rhel7/ci-build-debug/test_tools_co...
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
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@redhat.com Date: Tue, 24 Nov 2015 10:34:10 -0500 Subject: [PATCH 1/2] COLONDB: Add comments on functions
The colondb API provides three function:
- sss_colondb_open()
- sss_colondb_write_field()
- sss_colondb_read_field()
It is not obvious that sss_colondb_open() add destructor on talloc context which close the colondb during free context. And there is expectation that SSS_COLONDB_SENTINEL is type of last item in line.
So this patch adds simple lightening comments in doxygen style.
LGTM
From ab29fc0d22862bfd873f93982ba6a6ae9120f3b1 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@redhat.com Date: Fri, 27 Nov 2015 06:39:37 -0500 Subject: [PATCH 2/2] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
There are three functions at API of colondb wrapper:
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_writeline()
This patch adds tests for all of them.
We test those cases:
- open nonexisting file for read
- open nonexisting file for write
- open existing empty file for read
- open existing file with records for read
- open existing empty file for write
- open existing file with records for write
- write to empty file
- write to file with existing records
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_write_line()
- write to empty file and read it
Resolves: https://fedorahosted.org/sssd/ticket/2764
Makefile.am | 16 ++ src/tests/cmocka/test_tools_colondb.c | 423 ++++++++++++++++++++++++++++++++++ 2 files changed, 439 insertions(+) create mode 100644 src/tests/cmocka/test_tools_colondb.c
diff --git a/Makefile.am b/Makefile.am index a9d3f25d3775f6ac824b9f9b85dd0412417c33d3..12ac53d0e15ff1d62f36a8ed7b9898e23992927f 100644 --- a/Makefile.am +++ b/Makefile.am @@ -241,6 +241,7 @@ if HAVE_CMOCKA pam-srv-tests \ test_ipa_subdom_util \ test_ipa_subdom_server \
test_tools_colondb \ test_krb5_wait_queue \ test_cert_utils \ test_ldap_id_cleanup \@@ -2570,6 +2571,21 @@ test_ipa_subdom_server_LDADD = \ libdlopen_test_providers.la \ $(NULL)
+test_tools_colondb_SOURCES = \
- src/tests/cmocka/test_tools_colondb.c \
- src/tools/common/sss_colondb.c \
- $(NULL)
+test_tools_colondb_CFLAGS = \
- $(AM_CFLAGS) \
- $(NULL)
+test_tools_colondb_LDFLAGS = \
- $(NULL)
+test_tools_colondb_LDADD = \
- $(CMOCKA_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- libsss_test_common.la \
- $(NULL)
test_krb5_wait_queue_SOURCES = \ src/tests/cmocka/common_mock_be.c \ src/tests/cmocka/test_krb5_wait_queue.c \ diff --git a/src/tests/cmocka/test_tools_colondb.c b/src/tests/cmocka/test_tools_colondb.c new file mode 100644 index 0000000000000000000000000000000000000000..5b23e593add9349e701b977b134236234e6df773 --- /dev/null +++ b/src/tests/cmocka/test_tools_colondb.c @@ -0,0 +1,423 @@ +/*
- Authors:
Petr Čech <pcech@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+const char *TEST_STRING1 = "white"; +const int TEST_INT1 = 12;
+const char *TEST_STRING2 = "black"; +const int TEST_INT2 = 34;
+struct test_colondb_ctx {
- ;
+};
"test context" is usually used for passing values from setup funtion to tests.
If you plan to reuse in future in extended test then it might be easier to directly use TALLOC_CTX.
+static void create_dir(const char *path) +{
- errno_t ret;
- errno = 0;
- ret = mkdir(path, 0775);
- assert_int_equal(ret, 0);
+}
+static void create_empty_file(const char *path, const char *name) +{
- TALLOC_CTX *tmp_ctx = NULL;
- char *file_name = NULL;
- FILE *fp = NULL;
- tmp_ctx = talloc_new(NULL);
Please do not allocate anything on NULL context. It will disable leak checking leak_check_setup, leak_check_teardown Try to look into leak checking in test src/tests/cmocka/test_child_common.c. And it's not important whether allocation is in test or in setup function. It's better to be consistent,Otherwise patch LGTM.
There are few issues it the patch set. http://sssd-ci.duckdns.org/logs/job/36/89/summary.html
Linking error on debian. /usr/bin/ld: src/tests/cmocka/test_tools_colondb-test_tools_colondb.o: undefined reference to symbol 'poptFreeContext@@LIBPOPT_0' //lib/x86_64-linux-gnu/libpopt.so.0: error adding symbols: DSO missing from command line
just add $(POPT_LIBS) to test_tools_colondb_LDADD in Makefile.am
and also memory leak http://sssd-ci.duckdns.org/logs/job/36/89/rhel7/ci-build-debug/test_tools_co...
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@redhat.com Date: Thu, 18 Feb 2016 06:33:53 -0500 Subject: [PATCH 1/3] COLONDB: Fix memory leak
man 3 getline says: ssize_t getline(char **lineptr, size_t *n, FILE *stream); If *lineptr is set to NULL and *n is set 0 before the call, then getline() will allocate a buffer for storing the line. This buffer should be freed by the user program even if getline() failed.
This patch fix buffer freeing in case if getline() failed.
Nice catch.
ACK
From 016afc2aff7b980ba56690b6bafbcab377cf1e80 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@redhat.com Date: Tue, 24 Nov 2015 10:34:10 -0500 Subject: [PATCH 2/3] COLONDB: Add comments on functions
The colondb API provides three function:
- sss_colondb_open()
- sss_colondb_write_field()
- sss_colondb_read_field()
It is not obvious that sss_colondb_open() add destructor on talloc context which close the colondb during free context. And there is expectation that SSS_COLONDB_SENTINEL is type of last item in line.
So this patch adds simple lightening comments in doxygen style.
Resolves: https://fedorahosted.org/sssd/ticket/2764
ACK
From 036ffc7e61456baa9bbb112452bbcc483e0e07c3 Mon Sep 17 00:00:00 2001 From: Petr Cech pcech@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
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
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 Cechpcech@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 Cechpcech@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 Cechpcech@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
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 Slebodniklslebodn@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
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 Cechpcech@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 Cechpcech@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 Cechpcech@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
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 Slebodniklslebodn@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@redhat.com Date: Fri, 27 Nov 2015 06:39:37 -0500 Subject: [PATCH 4/4] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
There are three functions at API of colondb wrapper:
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_writeline()
This patch adds tests for all of them.
We test those cases:
- open nonexisting file for read
- open nonexisting file for write
- open existing empty file for read
- open existing file with records for read
- open existing empty file for write
- open existing file with records for write
- write to empty file
- write to file with existing records
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_write_line()
- write to empty file and read it
Resolves: https://fedorahosted.org/sssd/ticket/2764
Makefile.am | 17 ++ src/tests/cmocka/test_tools_colondb.c | 471 ++++++++++++++++++++++++++++++++++ 2 files changed, 488 insertions(+) create mode 100644 src/tests/cmocka/test_tools_colondb.c
diff --git a/Makefile.am b/Makefile.am index 8ff3322cd4aaaf8d419b664a18c4e0e67205beb8..4e4f38a5eaf5bfa2cfafa88b9b32848e6c27131b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -241,6 +241,7 @@ if HAVE_CMOCKA pam-srv-tests \ test_ipa_subdom_util \ test_ipa_subdom_server \
test_tools_colondb \ test_krb5_wait_queue \ test_cert_utils \ test_ldap_id_cleanup \@@ -2576,6 +2577,22 @@ test_ipa_subdom_server_LDADD = \ libdlopen_test_providers.la \ $(NULL)
+test_tools_colondb_SOURCES = \
- src/tests/cmocka/test_tools_colondb.c \
- src/tools/common/sss_colondb.c \
- $(NULL)
+test_tools_colondb_CFLAGS = \
- $(AM_CFLAGS) \
- $(NULL)
+test_tools_colondb_LDFLAGS = \
- $(NULL)
+test_tools_colondb_LDADD = \
- $(CMOCKA_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- $(POPT_LIBS) \
- libsss_test_common.la \
- $(NULL)
test_krb5_wait_queue_SOURCES = \ src/tests/cmocka/common_mock_be.c \ src/tests/cmocka/test_krb5_wait_queue.c \ diff --git a/src/tests/cmocka/test_tools_colondb.c b/src/tests/cmocka/test_tools_colondb.c new file mode 100644 index 0000000000000000000000000000000000000000..123ad39bd75a0206fbf152fd35a10c6113f36bfc --- /dev/null +++ b/src/tests/cmocka/test_tools_colondb.c @@ -0,0 +1,471 @@ +/*
- Authors:
Petr Čech <pcech@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+const char *TEST_STRING1 = "white"; +const int TEST_INT1 = 12;
+const char *TEST_STRING2 = "black"; +const int TEST_INT2 = 34;
+static void create_dir(const char *path) +{
- errno_t ret;
- errno = 0;
- ret = mkdir(path, 0775);
- assert_int_equal(ret, 0);
It's better to use assert_return_code for system calls. it will also print errno in cese of failure.
+}
+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
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 Cechpcech@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 Cechpcech@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 Cechpcech@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
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 Slebodniklslebodn@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@redhat.com Date: Fri, 27 Nov 2015 06:39:37 -0500 Subject: [PATCH 4/4] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
There are three functions at API of colondb wrapper:
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_writeline()
This patch adds tests for all of them.
We test those cases:
- open nonexisting file for read
- open nonexisting file for write
- open existing empty file for read
- open existing file with records for read
- open existing empty file for write
- open existing file with records for write
- write to empty file
- write to file with existing records
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_write_line()
- write to empty file and read it
Resolves: https://fedorahosted.org/sssd/ticket/2764
Makefile.am | 17 ++ src/tests/cmocka/test_tools_colondb.c | 471 ++++++++++++++++++++++++++++++++++ 2 files changed, 488 insertions(+) create mode 100644 src/tests/cmocka/test_tools_colondb.c
diff --git a/Makefile.am b/Makefile.am index 8ff3322cd4aaaf8d419b664a18c4e0e67205beb8..4e4f38a5eaf5bfa2cfafa88b9b32848e6c27131b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -241,6 +241,7 @@ if HAVE_CMOCKA pam-srv-tests \ test_ipa_subdom_util \ test_ipa_subdom_server \
test_tools_colondb \ test_krb5_wait_queue \ test_cert_utils \ test_ldap_id_cleanup \@@ -2576,6 +2577,22 @@ test_ipa_subdom_server_LDADD = \ libdlopen_test_providers.la \ $(NULL)
+test_tools_colondb_SOURCES = \
- src/tests/cmocka/test_tools_colondb.c \
- src/tools/common/sss_colondb.c \
- $(NULL)
+test_tools_colondb_CFLAGS = \
- $(AM_CFLAGS) \
- $(NULL)
+test_tools_colondb_LDFLAGS = \
- $(NULL)
+test_tools_colondb_LDADD = \
- $(CMOCKA_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- $(POPT_LIBS) \
- libsss_test_common.la \
- $(NULL)
test_krb5_wait_queue_SOURCES = \ src/tests/cmocka/common_mock_be.c \ src/tests/cmocka/test_krb5_wait_queue.c \ diff --git a/src/tests/cmocka/test_tools_colondb.c b/src/tests/cmocka/test_tools_colondb.c new file mode 100644 index 0000000000000000000000000000000000000000..123ad39bd75a0206fbf152fd35a10c6113f36bfc --- /dev/null +++ b/src/tests/cmocka/test_tools_colondb.c @@ -0,0 +1,471 @@ +/*
- Authors:
Petr Čech <pcech@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+const char *TEST_STRING1 = "white"; +const int TEST_INT1 = 12;
+const char *TEST_STRING2 = "black"; +const int TEST_INT2 = 34;
+static void create_dir(const char *path) +{
- errno_t ret;
- errno = 0;
- ret = mkdir(path, 0775);
- assert_int_equal(ret, 0);
It's better to use assert_return_code for system calls. it will also print errno in cese of failure.
Well.
+}
+static void create_empty_file(void **state, const char *path, const char *name) +{
- TALLOC_CTX *tmp_ctx = NULL;
- char *file_name = NULL;
- FILE *fp = NULL;
- tmp_ctx = talloc_new(*state);
^^^^^^ state was not initialized yet. and this function was called with "create_empty_file(state ...)" The stare is initialized at the end of "setup" function. Basically it means that you allocate on NULL context. So it would be better to use already created test_ctx in setup function which is protected with leak_check.
Good idea. I didn't recognize it.
- assert_non_null(tmp_ctx);
- create_dir(path);
- file_name = talloc_asprintf(tmp_ctx, "%s/%s", path, name);
- assert_non_null(file_name);
- fp = fopen(file_name, "w");
- assert_non_null(fp);
- fclose(fp);
- talloc_free(tmp_ctx);
+}
+static void create_nonempty_file(void **state,
const char *path, const char *name)+{
- TALLOC_CTX *tmp_ctx = NULL;
- struct sss_colondb *db = NULL;
- struct sss_colondb_write_field table[] = {
{ SSS_COLONDB_STRING, { .str = TEST_STRING2 } },{ SSS_COLONDB_UINT32, { .uint32 = TEST_INT2 } },{ SSS_COLONDB_SENTINEL, { 0 } }- };
- tmp_ctx = talloc_new(*state);
- assert_non_null(tmp_ctx);
- create_empty_file(state, TESTS_PATH, TESTS_FILE);
- db = sss_colondb_open(tmp_ctx, SSS_COLONDB_WRITE,
TESTS_PATH "/" TESTS_FILE);- assert_non_null(db);
- sss_colondb_writeline(db, table);
missing assert for writeline.
Nice catch.
- talloc_free(db);
- talloc_free(tmp_ctx);
+}
+static int setup(void **state, int file_state) +{
- TALLOC_CTX *test_ctx = NULL;
- assert_true(leak_check_setup());
- check_leaks_push(global_talloc_context);
- test_ctx = talloc_new(global_talloc_context);
- assert_non_null(test_ctx);
- switch (file_state) {
- case 0:
break;- case 1:
create_empty_file(state, TESTS_PATH, TESTS_FILE);break;- case 2:
create_nonempty_file(state, TESTS_PATH, TESTS_FILE);As I mentioned earlier, It would be good to paste "test_ctx" instead of state which is initialized before return.
break;- default:
break;- }
- *state = test_ctx;
BTW there is lot of code duplication in tests. There is check_leaks_push in the beginning and at the end of test.
If we move it to the end of setup function and to the beginning of teardown functions then test will be simpler.
the 1st check_leaks_push in setup will check for leaks in setup function and 2nd check_leaks_push will check for leaks in tests.
- return 0;
+}
+static int without_file_setup(void **state) +{
- return setup(state, 0);
+}
+static int with_empty_file_setup(void **state) +{
- return setup(state, 1);
+}
+static int with_nonempty_file_setup(void **state) +{
- return setup(state, 2);
+}
+static int teardown(void **state) +{
- errno_t ret;
- errno = 0;
- ret = unlink(TESTS_PATH "/" TESTS_FILE);
- if (ret != 0) {
assert_int_equal(errno, ENOENT);- }
- talloc_zfree(*state);
- test_dom_suite_cleanup(TESTS_PATH, NULL, NULL);
- assert_true(check_leaks_pop(global_talloc_context));
- assert_true(leak_check_teardown());
- return 0;
+}
//snip
+void test_read_from_nonempty(void **state) +{
- TALLOC_CTX *test_ctx = NULL;
- TALLOC_CTX *test_ctx_child = NULL;
- struct sss_colondb *db = NULL;
- errno_t ret;
- const char *string;
- uint32_t number;
- struct sss_colondb_read_field table[] = {
{ SSS_COLONDB_STRING, { .str = &string } },{ SSS_COLONDB_UINT32, { .uint32 = &number } },{ SSS_COLONDB_SENTINEL, { 0 } }- };
- test_ctx = talloc_new(*state);
- check_leaks_push(test_ctx);
- db = sss_colondb_open(test_ctx, SSS_COLONDB_READ,
TESTS_PATH "/" TESTS_FILE);- assert_non_null(db);
- test_ctx_child = talloc_new(test_ctx);
test_ctx_child is not required here. You just need to release "string" after readline. "test_ctx_child" could hide leaks in sss_colondb_readline.
Thanks, that was pain for me. test_ctx_child was hack.
- ret = sss_colondb_readline(test_ctx_child, db, table);
- assert_int_equal(ret, 0);
- assert_string_equal(string, TEST_STRING2);
- assert_int_equal(number, TEST_INT2);
- talloc_free(test_ctx_child);
- talloc_free(db);
- assert_true(check_leaks_pop(test_ctx));
+}
Attached diff should address all mentioned issues.
LS
Lukas, thanks for diff. I learned many thinks. New patches are attached.
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 Cechpcech@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 Cechpcech@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 Cechpcech@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
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 Slebodniklslebodn@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@redhat.com Date: Fri, 27 Nov 2015 06:39:37 -0500 Subject: [PATCH 4/4] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
There are three functions at API of colondb wrapper:
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_writeline()
This patch adds tests for all of them.
We test those cases:
- open nonexisting file for read
- open nonexisting file for write
- open existing empty file for read
- open existing file with records for read
- open existing empty file for write
- open existing file with records for write
- write to empty file
- write to file with existing records
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_write_line()
- write to empty file and read it
Resolves: https://fedorahosted.org/sssd/ticket/2764
Makefile.am | 17 ++ src/tests/cmocka/test_tools_colondb.c | 471 ++++++++++++++++++++++++++++++++++ 2 files changed, 488 insertions(+) create mode 100644 src/tests/cmocka/test_tools_colondb.c
diff --git a/Makefile.am b/Makefile.am index 8ff3322cd4aaaf8d419b664a18c4e0e67205beb8..4e4f38a5eaf5bfa2cfafa88b9b32848e6c27131b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -241,6 +241,7 @@ if HAVE_CMOCKA pam-srv-tests \ test_ipa_subdom_util \ test_ipa_subdom_server \
test_tools_colondb \ test_krb5_wait_queue \ test_cert_utils \ test_ldap_id_cleanup \@@ -2576,6 +2577,22 @@ test_ipa_subdom_server_LDADD = \ libdlopen_test_providers.la \ $(NULL)
+test_tools_colondb_SOURCES = \
- src/tests/cmocka/test_tools_colondb.c \
- src/tools/common/sss_colondb.c \
- $(NULL)
+test_tools_colondb_CFLAGS = \
- $(AM_CFLAGS) \
- $(NULL)
+test_tools_colondb_LDFLAGS = \
- $(NULL)
+test_tools_colondb_LDADD = \
- $(CMOCKA_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- $(POPT_LIBS) \
- libsss_test_common.la \
- $(NULL)
test_krb5_wait_queue_SOURCES = \ src/tests/cmocka/common_mock_be.c \ src/tests/cmocka/test_krb5_wait_queue.c \ diff --git a/src/tests/cmocka/test_tools_colondb.c b/src/tests/cmocka/test_tools_colondb.c new file mode 100644 index 0000000000000000000000000000000000000000..123ad39bd75a0206fbf152fd35a10c6113f36bfc --- /dev/null +++ b/src/tests/cmocka/test_tools_colondb.c @@ -0,0 +1,471 @@ +/*
- Authors:
Petr Čech <pcech@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+const char *TEST_STRING1 = "white"; +const int TEST_INT1 = 12;
+const char *TEST_STRING2 = "black"; +const int TEST_INT2 = 34;
+static void create_dir(const char *path) +{
- errno_t ret;
- errno = 0;
- ret = mkdir(path, 0775);
- assert_int_equal(ret, 0);
It's better to use assert_return_code for system calls. it will also print errno in cese of failure.
Well.
+}
+static void create_empty_file(void **state, const char *path, const char *name) +{
- TALLOC_CTX *tmp_ctx = NULL;
- char *file_name = NULL;
- FILE *fp = NULL;
- tmp_ctx = talloc_new(*state);
^^^^^^ state was not initialized yet. and this function was called with "create_empty_file(state ...)" The stare is initialized at the end of "setup" function. Basically it means that you allocate on NULL context. So it would be better to use already created test_ctx in setup function which is protected with leak_check.Good idea. I didn't recognize it.
- assert_non_null(tmp_ctx);
- create_dir(path);
- file_name = talloc_asprintf(tmp_ctx, "%s/%s", path, name);
- assert_non_null(file_name);
- fp = fopen(file_name, "w");
- assert_non_null(fp);
- fclose(fp);
- talloc_free(tmp_ctx);
+}
+static void create_nonempty_file(void **state,
const char *path, const char *name)+{
- TALLOC_CTX *tmp_ctx = NULL;
- struct sss_colondb *db = NULL;
- struct sss_colondb_write_field table[] = {
{ SSS_COLONDB_STRING, { .str = TEST_STRING2 } },{ SSS_COLONDB_UINT32, { .uint32 = TEST_INT2 } },{ SSS_COLONDB_SENTINEL, { 0 } }- };
- tmp_ctx = talloc_new(*state);
- assert_non_null(tmp_ctx);
- create_empty_file(state, TESTS_PATH, TESTS_FILE);
- db = sss_colondb_open(tmp_ctx, SSS_COLONDB_WRITE,
TESTS_PATH "/" TESTS_FILE);- assert_non_null(db);
- sss_colondb_writeline(db, table);
missing assert for writeline.Nice catch.
- talloc_free(db);
- talloc_free(tmp_ctx);
+}
+static int setup(void **state, int file_state) +{
- TALLOC_CTX *test_ctx = NULL;
- assert_true(leak_check_setup());
- check_leaks_push(global_talloc_context);
- test_ctx = talloc_new(global_talloc_context);
- assert_non_null(test_ctx);
- switch (file_state) {
- case 0:
break;- case 1:
create_empty_file(state, TESTS_PATH, TESTS_FILE);break;- case 2:
create_nonempty_file(state, TESTS_PATH, TESTS_FILE);As I mentioned earlier, It would be good to paste "test_ctx" instead of state which is initialized before return.
break;- default:
break;- }
- *state = test_ctx;
BTW there is lot of code duplication in tests. There is check_leaks_push in the beginning and at the end of test.
If we move it to the end of setup function and to the beginning of teardown functions then test will be simpler.
the 1st check_leaks_push in setup will check for leaks in setup function and 2nd check_leaks_push will check for leaks in tests.
- return 0;
+}
+static int without_file_setup(void **state) +{
- return setup(state, 0);
+}
+static int with_empty_file_setup(void **state) +{
- return setup(state, 1);
+}
+static int with_nonempty_file_setup(void **state) +{
- return setup(state, 2);
+}
+static int teardown(void **state) +{
- errno_t ret;
- errno = 0;
- ret = unlink(TESTS_PATH "/" TESTS_FILE);
- if (ret != 0) {
assert_int_equal(errno, ENOENT);- }
- talloc_zfree(*state);
- test_dom_suite_cleanup(TESTS_PATH, NULL, NULL);
- assert_true(check_leaks_pop(global_talloc_context));
- assert_true(leak_check_teardown());
- return 0;
+}
//snip
+void test_read_from_nonempty(void **state) +{
- TALLOC_CTX *test_ctx = NULL;
- TALLOC_CTX *test_ctx_child = NULL;
- struct sss_colondb *db = NULL;
- errno_t ret;
- const char *string;
- uint32_t number;
- struct sss_colondb_read_field table[] = {
{ SSS_COLONDB_STRING, { .str = &string } },{ SSS_COLONDB_UINT32, { .uint32 = &number } },{ SSS_COLONDB_SENTINEL, { 0 } }- };
- test_ctx = talloc_new(*state);
- check_leaks_push(test_ctx);
- db = sss_colondb_open(test_ctx, SSS_COLONDB_READ,
TESTS_PATH "/" TESTS_FILE);- assert_non_null(db);
- test_ctx_child = talloc_new(test_ctx);
test_ctx_child is not required here. You just need to release "string" after readline. "test_ctx_child" could hide leaks in sss_colondb_readline.
Thanks, that was pain for me. test_ctx_child was hack.
- ret = sss_colondb_readline(test_ctx_child, db, table);
- assert_int_equal(ret, 0);
- assert_string_equal(string, TEST_STRING2);
- assert_int_equal(number, TEST_INT2);
- talloc_free(test_ctx_child);
- talloc_free(db);
- assert_true(check_leaks_pop(test_ctx));
+}
Attached diff should address all mentioned issues.
LS
Lukas, thanks for diff. I learned many thinks. New patches are attached.
ACK
http://sssd-ci.duckdns.org/logs/job/37/88/summary.html
LS
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 Cechpcech@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 Cechpcech@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 Cechpcech@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 Slebodniklslebodn@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@redhat.com Date: Fri, 27 Nov 2015 06:39:37 -0500 Subject: [PATCH 4/4] TEST_TOOLS_COLONDB: Add tests for sss_colondb_*
There are three functions at API of colondb wrapper:
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_writeline()
This patch adds tests for all of them.
We test those cases:
- open nonexisting file for read
- open nonexisting file for write
- open existing empty file for read
- open existing file with records for read
- open existing empty file for write
- open existing file with records for write
- write to empty file
- write to file with existing records
- sss_colondb_open()
- sss_colondb_readline()
- sss_colondb_write_line()
- write to empty file and read it
Resolves: https://fedorahosted.org/sssd/ticket/2764
Makefile.am | 17 ++ src/tests/cmocka/test_tools_colondb.c | 471 ++++++++++++++++++++++++++++++++++ 2 files changed, 488 insertions(+) create mode 100644 src/tests/cmocka/test_tools_colondb.c
diff --git a/Makefile.am b/Makefile.am index 8ff3322cd4aaaf8d419b664a18c4e0e67205beb8..4e4f38a5eaf5bfa2cfafa88b9b32848e6c27131b 100644 --- a/Makefile.am +++ b/Makefile.am @@ -241,6 +241,7 @@ if HAVE_CMOCKA pam-srv-tests \ test_ipa_subdom_util \ test_ipa_subdom_server \
test_tools_colondb \ test_krb5_wait_queue \ test_cert_utils \ test_ldap_id_cleanup \@@ -2576,6 +2577,22 @@ test_ipa_subdom_server_LDADD = \ libdlopen_test_providers.la \ $(NULL)
+test_tools_colondb_SOURCES = \
- src/tests/cmocka/test_tools_colondb.c \
- src/tools/common/sss_colondb.c \
- $(NULL)
+test_tools_colondb_CFLAGS = \
- $(AM_CFLAGS) \
- $(NULL)
+test_tools_colondb_LDFLAGS = \
- $(NULL)
+test_tools_colondb_LDADD = \
- $(CMOCKA_LIBS) \
- $(SSSD_INTERNAL_LTLIBS) \
- $(POPT_LIBS) \
- libsss_test_common.la \
- $(NULL)
test_krb5_wait_queue_SOURCES = \ src/tests/cmocka/common_mock_be.c \ src/tests/cmocka/test_krb5_wait_queue.c \ diff --git a/src/tests/cmocka/test_tools_colondb.c b/src/tests/cmocka/test_tools_colondb.c new file mode 100644 index 0000000000000000000000000000000000000000..123ad39bd75a0206fbf152fd35a10c6113f36bfc --- /dev/null +++ b/src/tests/cmocka/test_tools_colondb.c @@ -0,0 +1,471 @@ +/*
- Authors:
Petr Čech <pcech@redhat.com>- Copyright (C) 2015 Red Hat
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 3 of the License, or
- (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program. If not, see http://www.gnu.org/licenses/.
+*/
+#include <talloc.h> +#include <errno.h> +#include <popt.h>
+#include "tests/cmocka/common_mock.h" +#include "src/tools/common/sss_colondb.h"
+#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TESTS_FILE "test_colondb.ldb"
+const char *TEST_STRING1 = "white"; +const int TEST_INT1 = 12;
+const char *TEST_STRING2 = "black"; +const int TEST_INT2 = 34;
+static void create_dir(const char *path) +{
- errno_t ret;
- errno = 0;
- ret = mkdir(path, 0775);
- assert_int_equal(ret, 0);
It's better to use assert_return_code for system calls. it will also print errno in cese of failure.
Well.
+}
+static void create_empty_file(void **state, const char *path, const char *name) +{
- TALLOC_CTX *tmp_ctx = NULL;
- char *file_name = NULL;
- FILE *fp = NULL;
- tmp_ctx = talloc_new(*state);
^^^^^^ state was not initialized yet. and this function was called with "create_empty_file(state ...)" The stare is initialized at the end of "setup" function. Basically it means that you allocate on NULL context. So it would be better to use already created test_ctx in setup function which is protected with leak_check.Good idea. I didn't recognize it.
- assert_non_null(tmp_ctx);
- create_dir(path);
- file_name = talloc_asprintf(tmp_ctx, "%s/%s", path, name);
- assert_non_null(file_name);
- fp = fopen(file_name, "w");
- assert_non_null(fp);
- fclose(fp);
- talloc_free(tmp_ctx);
+}
+static void create_nonempty_file(void **state,
const char *path, const char *name)+{
- TALLOC_CTX *tmp_ctx = NULL;
- struct sss_colondb *db = NULL;
- struct sss_colondb_write_field table[] = {
{ SSS_COLONDB_STRING, { .str = TEST_STRING2 } },{ SSS_COLONDB_UINT32, { .uint32 = TEST_INT2 } },{ SSS_COLONDB_SENTINEL, { 0 } }- };
- tmp_ctx = talloc_new(*state);
- assert_non_null(tmp_ctx);
- create_empty_file(state, TESTS_PATH, TESTS_FILE);
- db = sss_colondb_open(tmp_ctx, SSS_COLONDB_WRITE,
TESTS_PATH "/" TESTS_FILE);- assert_non_null(db);
- sss_colondb_writeline(db, table);
missing assert for writeline.Nice catch.
- talloc_free(db);
- talloc_free(tmp_ctx);
+}
+static int setup(void **state, int file_state) +{
- TALLOC_CTX *test_ctx = NULL;
- assert_true(leak_check_setup());
- check_leaks_push(global_talloc_context);
- test_ctx = talloc_new(global_talloc_context);
- assert_non_null(test_ctx);
- switch (file_state) {
- case 0:
break;- case 1:
create_empty_file(state, TESTS_PATH, TESTS_FILE);break;- case 2:
create_nonempty_file(state, TESTS_PATH, TESTS_FILE);As I mentioned earlier, It would be good to paste "test_ctx" instead of state which is initialized before return.
break;- default:
break;- }
- *state = test_ctx;
BTW there is lot of code duplication in tests. There is check_leaks_push in the beginning and at the end of test.
If we move it to the end of setup function and to the beginning of teardown functions then test will be simpler.
the 1st check_leaks_push in setup will check for leaks in setup function and 2nd check_leaks_push will check for leaks in tests.
- return 0;
+}
+static int without_file_setup(void **state) +{
- return setup(state, 0);
+}
+static int with_empty_file_setup(void **state) +{
- return setup(state, 1);
+}
+static int with_nonempty_file_setup(void **state) +{
- return setup(state, 2);
+}
+static int teardown(void **state) +{
- errno_t ret;
- errno = 0;
- ret = unlink(TESTS_PATH "/" TESTS_FILE);
- if (ret != 0) {
assert_int_equal(errno, ENOENT);- }
- talloc_zfree(*state);
- test_dom_suite_cleanup(TESTS_PATH, NULL, NULL);
- assert_true(check_leaks_pop(global_talloc_context));
- assert_true(leak_check_teardown());
- return 0;
+}
//snip
+void test_read_from_nonempty(void **state) +{
- TALLOC_CTX *test_ctx = NULL;
- TALLOC_CTX *test_ctx_child = NULL;
- struct sss_colondb *db = NULL;
- errno_t ret;
- const char *string;
- uint32_t number;
- struct sss_colondb_read_field table[] = {
{ SSS_COLONDB_STRING, { .str = &string } },{ SSS_COLONDB_UINT32, { .uint32 = &number } },{ SSS_COLONDB_SENTINEL, { 0 } }- };
- test_ctx = talloc_new(*state);
- check_leaks_push(test_ctx);
- db = sss_colondb_open(test_ctx, SSS_COLONDB_READ,
TESTS_PATH "/" TESTS_FILE);- assert_non_null(db);
- test_ctx_child = talloc_new(test_ctx);
test_ctx_child is not required here. You just need to release "string" after readline. "test_ctx_child" could hide leaks in sss_colondb_readline.
Thanks, that was pain for me. test_ctx_child was hack.
- ret = sss_colondb_readline(test_ctx_child, db, table);
- assert_int_equal(ret, 0);
- assert_string_equal(string, TEST_STRING2);
- assert_int_equal(number, TEST_INT2);
- talloc_free(test_ctx_child);
- talloc_free(db);
- assert_true(check_leaks_pop(test_ctx));
+}
Attached diff should address all mentioned issues.
LS
Lukas, thanks for diff. I learned many thinks. New patches are attached.
ACK
master: * b590f44c06158485357d69cc5b24d5af05f1bb95 * cf1109e30320a994187edeb438ac7cdc36f0dd2b * 2dd75ea79a57615808754c0ce550786edbc17d69 * 6977d7c84145ac69195be58b3330861b9b8a3b72
sssd-1-13: * b269edafff139510ee1e9c00bdbc8f27e8aea691 * fbf7d5683287fa2c7b450b8f5b0df63673f25d83 * 34ba0c53d0d966c64ea11a6269cdd0ad985f4068 * d75ac50d0c065974a7ec2330f60657ae85e487c0
LS
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Petr Cech