TALLOC: Discussion of destructor processing

[simo]

On Wed, 2012-05-02 at 09:05 +0200, Stefan (metze) Metzmacher wrote: 
> Hi,
> 
> a return of -1 from a destructor doesn't mean it "failed",
> it means that it has a very good reason to reject a talloc_free() of all
> it's children
> and the free() on itself. It's the duty of the destructor logic to
> prevent memory leaks!

Metze, excellent explanation all around.
I wonder if adding 2 defines like:
#define TD_ALL_OK 0
#define TD_DONT_FREE -1
and use them instead of the bare numbers together with some explanation
in the docs taken from this mail wouldn't improve a lot other people
understanding.

Simo.


> Typically the caller doesn't need to do need to check the return value
> of talloc_free(),
> as from the caller's perspective the memory is gone and it should not
> care if the destructor
> wants to do anything special. The case where checking the return of
> talloc_free() make sense
> is when the caller exactly knows what the specific destructor is doing.
> 
> In samba we have a few places where we rely on that feature, e.g. see
> tevent_common_timed_deny_destructor(), dcerpc_connection_destructor(),
> packet_destructor(), smb2srv_request_deny_destructor(),
> lpcfg_destructor(), system_session_destructor()
> 
> store_destructor in paged_results.c seems to be a bug, I think there's
> no reason
> to return -1 there. The same for db_ctdb_transaction_destructor().
> 
> Any complex operation that can fail and should be avoided and
> a failing logic inside a destructor should not result in a return of -1.
> 
> The tstream_cli_np_destructor() example Volker mentioned, is in its
> current state
> because it was like that before. The correct implementation would trigger an
> async close, let the tstream_cli_np become a talloc child of cli_state,
> set the destructor to NULL and return -1. The callback function triggered
> by the async close can call talloc_free() without destructor then.
> Or it gets free'ed by a talloc_free(cli_state) by a higher level caller.
> 
> I think the only thing we should do here is to enhance the documentation.
> 
> And if you don't want to care about all this in your application, just only
> use simple destructors and always return 0.
> 
> metze


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