[SSSD] Re: [PATCH SET] TEST_TOOLS_COLONDB: Add tests for sss_colondb_* public API

[Petr Cech]

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 Cech
> > > 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 Cech
> > > 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 Cech
> > > 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 Slebodnik
> 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