ldb_search interface considered harmful

[simo]

On Wed, 2006-02-01 at 07:41 +1100, Andrew Bartlett wrote:
> On Tue, 2006-01-31 at 09:18 -0500, simo wrote:
> > On Tue, 2006-01-31 at 16:02 +1100, Andrew Bartlett wrote:
> > > I've recently been working on the Samba4 code, and the output of
> > > --leak-report-full in particular.
> > > 
> > > The contents of this (for an inactive connection) should be quite small,
> > > but I initially found a large number of ldb_message structures being
> > > kept around.  While some of this was easy to clean up, the problem
> > > appears to be the interface:
> > > 
> > > int ldb_search(struct ldb_context *ldb,
> > >                const struct ldb_dn *base,
> > >                enum ldb_scope scope,
> > >                const char *expression,
> > >                const char * const *attrs, struct ldb_result **res);
> > > 
> > > Because the *res is returned on the ldb as a context, it often stays
> > > there.  The gendb wrapper 'fixes' this, but we don't always use it.
> > > Should we modify the interface to have an explicit mem_ctx argument, to
> > > avoid the caller needing to manually talloc_steal?
> > 
> > I would like to avoid passing down a mem_ctx and just document the fact.
> > The API is made to be usable in any program and probably avoiding
> > programs to be forced into using talloc is preferable.
> > You should always free the result once done with a call, and talloc
> > should then free everything down that path.
> 
> If the caller has to call talloc_free(), doesn't that mean that they
> must know about talloc()?

ops right, I forgot we removed the ldb routines to free the result when
we moved ldb to talloc.

> The problem I have is that the default behaviour is to leak memory onto
> the long-term context.  Everything continues to work perfectly, but
> memory leaks.  Interfaces should be designed so that the easiest way to
> code them is also the correct one.  I assert (by handwaving) that
> calling both ldb_search and talloc_steal (on success) isn't the easiest
> way to use the interface, and therefore callers will get it wrong.

Well the memory should be all dependent on the result, so forgetting to
free the result is the real problem. It's like passing a memory context
and forgetting to free it.

Are you proposing to just add a mem_ctx parm to ldb_search and
talloc_steal in ldb_search() or are you proposing to pass down this
mem_ctx to the backend modules through ldb_request ?

Simo.

-- 
Simo Sorce
Samba Team
email: idra at samba.org
http://samba.org/~idra