[QUICK] talloc bugs

[simo]

On Wed, 2009-07-01 at 21:20 +0930, Rusty Russell wrote:
> On Mon, 29 Jun 2009 09:19:20 pm tridge at samba.org wrote:
> > Hi Rusty,
> >
> >  > I'd like to see a simple example of where talloc_reference is required,
> >  > so we can get less abstract in this discussion.
> >
> > yep, I've found the abstractness of the discussion unhelpful.
> >
> > Here are two fairly simple examples:
> >
> >  1) in source3/lib/util_tdb.c we have tdb_wrap_open() which is used to
> >  allow us to share a common underlying tdb context between two users,
> >  allowing for the illusion of being able to open a tdb more than once
> >  in the same process. This is a classic case of reference counting.
> 
> This is a classic case of problematic references, too.  Ignoring the fact that 
> TDB should handle multiple opens, there's nothing obviously wrong with:
> 
> 	tdb = tdb_wrap_open(...);
> 	talloc_free(tdb);
> 
> Nor:
> 	tdb = tdb_wrap_open(...);
> 	talloc_steal(newctx, tdb);
> 
> Yet both are buggy, because the reference is *exposed*.  The right answer is 
> to hide it:
> 
> util_tdb.h:
> struct tdb_wrap {
> 	struct tdb_context *tdb;
> 	struct tdb_wrap_info *wi;
> };
> 
> util_tdb.c:
> struct tdb_wrap_info {
> 	struct tdb_wrap_info *next, *prev;
> 	const char *name; /* Redundant, use tdb_name */
> 	struct tdb_context *tdb;
> };
> 
> Once this is done, it's a short hop to open-coding the reference count anyway 
> rather than using talloc_reference.
> 
> I don't think references are a fundamental part of talloc: they are useful, 
> and I think helpers to allow them could be great, but I'd rather see some 
> explicit demarcation of what is reference counted.  See followup code.

Another way could be to attach some code like we do with
talloc_destructor(), with the ability to tell the calling talloc code to
reparent instead of free.

But KISS in this case is probably the better approach and simply
deprecating talloc_reference() and using explicit references as above
may just suffice.

In any case I'd like to stress we need to maintain ABI compatibility, so
whatever the decision I think we need still quite some work on what was
currently committed (possibly with a revert first).

Simo.

-- 
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Principal Software Engineer at Red Hat, Inc. <simo at redhat.com>