TALLOC: Discussion of destructor processing

[Jeremy Allison]

On Mon, Apr 30, 2012 at 01:38:35PM -0400, Stephen Gallagher wrote:
> I was having a discussion with Pavel Březina about talloc destructors
> and he made me aware of the underlying algorithm used to process them
> during a nested free operation.
> 
> So, to summarize quickly, the behavior is as follows: (pseudo-code).
> 
> if (destructor(ctx) == -1) {
>      return -1;
> }
> free_children(ctx); /* This recurses */
> free(ctx);
> 
> 
> I see several problems with this approach.
> 
> 1) The results of the destructors for the children are not considered
> when determining whether to report success or failure for the
> talloc_free(). This means that if any part of the hierarchy except the
> top-most context is freed, there will be a memory leak (with probably
> unreachable memory).
> 
> 2) It strikes me as strange that we're calling the destructor before
> we're we're handling the destructors of the children. It's contrary to
> (at least my) expectations about the order that destructors should
> execute. Freeing of memory and execution of destructors should be
> happening from the "bottom-up" of the hierarchy.
> 
> 
> My vision of how this should behave is as follows: We should iterate
> through the hierarchy tree, starting from the leaves. Work its way back
> up, calling destructors where they exist until either we have called all
> of them or any one has failed. If any failed, return -1 to the top-level
> free. If all succeeded, THEN iterate the tree a second time to actually
> perform the deallocation.

To be honest that sounds so reasonable I kind of assumed it was
the way we were already doing it :-).

I can't think of any code that's dependend on this current
destructor order, but that's not to say there isn't any in
Samba.

What would help is a patch implementing your desired semantics
and then we can examine the effects it has on the current code
with valgrind and other tools.

Cheers,

	Jeremy.