[PATCH] new macro: talloc_ctxdup

[simo]

On Tue, 2012-05-01 at 07:40 -0400, Stephen Gallagher wrote: 
> On Tue, 2012-05-01 at 21:26 +1000, Andrew Bartlett wrote:
> > On Tue, 2012-05-01 at 07:12 -0400, Stephen Gallagher wrote:
> ...
> > > This patch came out of a discussion that Pavel and I were having about
> > > copying memory out of a talloc pool into a new context. This seemed an
> > > expedient way to do it. Obviously as Andrew points out, you need to also
> > > be aware of whether the children of this context are also allocated from
> > > the pool.
> > > 
> > > Perhaps it would be better to design a talloc_steal_ext() that could be
> > > configured to recursively move child memory out of a pool, where
> > > appropriate.
> > 
> > I'm a little confused, as talloc_steal() already moves the context and
> > any child memory to the new parent.  What would your talloc_steal_ext()
> > do?
> 
> Actually, that's a false assumption. In the following construct:
> 
> TALLOC_CTX *tmp_ctx = talloc_pool(NULL, 65536);
> struct user *user = talloc(tmp_ctx, struct user);
> user->username = talloc_strdup(user, "user1");
> 
> Ok, so now we have 64k of memory allocated to a pool. We've taken some
> portion of it for use with user and its username.
> 
> Now:
> new_user = talloc_steal(mem_ctx, user);
> This reassigns the parent to the mem_ctx, but the memory is still
> allocated in the pool.
> 
> talloc_free(tmp_ctx); /* This doesn't do what you think it does */
> 
> The memory has its PARENTAGE as mem_ctx now, but the pool itself will
> not actually be freed until such time as the new_user is freed (and by
> extension new_user->username). So in the meantime, we're holding on to
> the entire 64k pool memory.
> 
> The idea behind talloc_steal_ext() would be that under the hood it would
> iterate through and determine if any part of the memory was in a
> talloc_pool. If so, it would perform a talloc_ctxdup() to reallocate it
> on the new context hierarchy (which may or may not be a NEW
> talloc_pool). The end result would be that the original talloc_pool
> could then be freed immediately, instead of hanging on to potentially
> large amounts of memory that would look like a memory leak without being
> detectable as one by valgrind.

In this case I would rather either do that by default or maybe
distinguish between talloc_steal() (does not copy enything out of the
pool) and talloc_move() (where I would do the actual move of memory out
of the pool).

However what I need to understand is where is the problem in keeping
around a talloc_pool. The point of talloc_pool is to make
allocations/deallocation faster, and should be used in fast paths, not
everywhere.
My sense is that if you need to use talloc_steal() it means that you
shouldn't use a pool.

FWIW: I also agree that talloc_dupctx() is a confusing name and most
iomportantly and easy way to fail to copy around memory properly, and
because we cannot easily cioy a hierarchy w/o being aware of the actual
structure used I thik I would rather not introduce this function, not
with this name at least,
If you want to call it talloc_named_memdup() I's be ok with it though.

Simo.

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