On (12/07/16 14:08), Fabiano FidĂȘncio wrote:
On Tue, Jul 12, 2016 at 2:06 PM, Lukas Slebodnik
<lslebodn(a)redhat.com> 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?
LS