LDB hidden memory leaks

[Kamen Mazdrashki]

Hi Tridge,

On Fri, Jul 16, 2010 at 07:47, <tridge at samba.org> wrote:

> Hi Kamen,
>
> Andrew and I looked at your ldb talloc changes, and they look good
> except for two little things:
>

>  1) you put the talloc context as the 3rd argument in the new
>  functions. The convention we have used (almost) everywhere in Samba
>  is that the first argument is the main context structure, and the 2nd
>  argument is the talloc context for the result. Other arguments follow
>  those. I think making these functions put the talloc context 3rd may
>  lead to more typos in future (as TALLOC_CTX is a void, we get no type
>  checking on this, so conventions are all we have).
>
>  2) the (TALLOC_CTX *) cast also goes against our established
>  convention. If we were to move to doing that, I think it may be
>  better to use something like:
>
>   #define TC(x) x
>
>  and use TC(ptr) as a visual marker.
>
>  Don't do that now though - if we decide to adopt TALLOC_CTX casting,
>  let's do it as a separate patch.
>
> Note that we do have some functions in ldb that don't follow the
> "context then TALLOC_CTX then args" convention (eg. some ldb_dn
> functions), but I don't think we have any with TALLOC_CTX 3rd. So
> while we haven't been entirely consistent, introducing a 3rd varient
> on argument ordering is asking for trouble :-)
>
> Cheers, Tridge
>


Actually, those two things are that little :)
As the driving force for this patch was "making better interfaces" those
two things are a big part of what I was trying to start.

Which was:
1) pass TALLOC_CTX explicitly as func parameter - so it is clear who
the owner of the memory is

2) make func prototype (input params, mem context, output params) form
in public functions. Basically "output params are last" rule.
I started this as I was quite confused  to read code like this:
ret = ldb_do_something(ldb, ldb, value, msg) - looks kind of strange :)

3) explicitly cast the pointer used as memory context to TALLOC_CTX
in callers - so it is instantly visible which is memory context.
This kind of solves 2).

I was kind of hopping that either 2) or 3) will be accepted  - so I put
them
both ready to sacrifice on of them :). Refusing both of them doesn't make
me that happy with my patch actually.

And while 2) is matter of taste, 3) is a real development aid, so I want to
appeal to the team to consider adopting some kind of visual markers
for memory contexts in the future.
The proposal you gave is definitely a better one than plain casts.

Anyway, I will fix the ldb-mem-fix branch shorty.

As a final note:
I think that aids like this will give novice samba developers (like me)
an easier start. We need more developers and I seriously doubt
those new guys will be "10 years of experience in open source"
type of people :)

-- 
CU,
Kamen Mazdrashki
Samba Team                                            http://samba.org
http://gitweb.samba.org/?p=kamenim/samba.git;a=summary