On Mon, Jul 18, 2016 at 2:53 PM, Lukas Slebodnik <[email protected]> wrote: > On (12/07/16 14:08), Fabiano Fidêncio wrote: >>On Tue, Jul 12, 2016 at 2:06 PM, Lukas Slebodnik <[email protected]> wrote: >>> On (12/07/16 13:57), Fabiano Fidêncio wrote: >>>>Comments are in-line. >>>> >>>>The test directory was not removed (tp_test_utils-test_utils) >>>>because it contain the snippet for krb5_libdefaults. >>>>--- >>>> src/tests/cmocka/test_utils.c | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>>diff --git a/src/tests/cmocka/test_utils.c b/src/tests/cmocka/test_utils.c >>>>index >>>>fd20990ce7ac632b3b62bf84a20cc75a5ec0e968..b08b19708bb59a076a79805fa37a15924152b8e2 >>>>100644 >>>>--- a/src/tests/cmocka/test_utils.c >>>>+++ b/src/tests/cmocka/test_utils.c >>>>@@ -1252,6 +1252,7 @@ void test_sss_write_krb5_conf_snippet(void **state) >>>> char *cwd; >>>> char *path; >>>> char *file; >>>>+ char *file_krb5_libdefaults; >>>> >>>> ret = sss_write_krb5_conf_snippet(NULL, false); >>>> assert_int_equal(ret, EINVAL); >>>>@@ -1274,6 +1275,10 @@ void test_sss_write_krb5_conf_snippet(void **state) >>>> ret = asprintf(&file, "%s/%s/localauth_plugin", cwd, TESTS_PATH); >>>> assert_true(ret > 0); >>>> >>>>I know it's not part of your change and I know it's just in the test >>>>suite, but by doing the assert here you're leaking the path. >>>> >>>>+ ret = asprintf(&file_krb5_libdefaults, >>>>+ "%s/%s/krb5_libdefaults", cwd, TESTS_PATH); >>>>+ assert_true(ret > 0); >>>>+ >>>> >>>>Leaking path and file here. >>>> >>>> ret = sss_write_krb5_conf_snippet(path, true); >>>> assert_int_equal(ret, EOK); >>>> >>>>@@ -1286,7 +1291,11 @@ void test_sss_write_krb5_conf_snippet(void **state) >>>> assert_int_equal(ret, EOK); >>>> #endif >>>> >>>>+ ret = unlink(file_krb5_libdefaults); >>>>+ assert_int_equal(ret, EOK); >>>> >>>>Leaking the path, file and file_krb6_libdefaults here.. >>>> >>> We do not leak memory if test passes >> >>Yep, the leaks can happen only in case of failure. >> >>> and we have a requirement that unit test have to pass with each commit. >>> >>> We could write a setup and teardown to get rid of potential leaks >>> in case of failure. But IMHO it does not worth it. >> >>Indeed! > Does it mean ACK?
Yes, it does. Sorry for not being explicit. I will be, next time. Best Regards, -- Fabiano Fidêncio _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
