On (18/07/16 14:56), Fabiano Fidêncio wrote: >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. > http://sssd-ci.duckdns.org/logs/job/49/90/summary.html
master: * 059904af2d20debcb8ffe1c6f45b996c2c57574e LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
