[PATCH] new macro: talloc_ctxdup

[Volker Lendecke]

On Tue, May 01, 2012 at 07:40:33AM -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.

Just don't allocate long-living memory from a potentially
short-lived talloc_pool. The talloc_pool API was written
because during the pstring removal for file names we had
measurably lost performance, and malloc was very high on the
profiles. talloc_pool brought that back, so I think for the
normal SMB use it is a valid optimization. But you have to
be careful not to allocate memory you later talloc_steal to
some long-living context. It does work (if it doesn't please
tell me!), but it has the drawback you describe.

With best regards,

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de