[PATCH] new macro: talloc_ctxdup

[Pavel Březina]

On 05/01/2012 02:54 PM, simo wrote:
> 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.

Sounds reasonable.

> 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.

Patch is attached.

Pavel.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-added-talloc_named_memdup.patch
Type: text/x-patch
Size: 1961 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20120502/ca532bec/attachment.bin>