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