On 04/29/2010 09:47 PM, Simo Sorce wrote:
On Mon, 19 Apr 2010 15:19:22 +0400
Eugene Indenbom<eindenbom(a)gmail.com> wrote:
> On 04/16/2010 10:22 PM, Simo Sorce wrote:
>
>> On Fri, 16 Apr 2010 19:00:57 +0400
>> Eugene Indenbom<eindenbom(a)gmail.com> wrote:
>>
>>
>>
>>> Looks equivalent to what you have written, a bit more verbose may
>>> be.
>>>
>> Ok in this case I think your patch is overkill.
>> All we really need is to use the destructor to do what it is meant
>> for, and just prevent freeing sdap_handle if we are recursing, and
>> at the same time use a lock so that only one sdap_handle_release
>> (the ""outermost"" does the job).
>>
>> Attached find a patch.
>>
>>
> Very clever solution. Actually I am pretty much surprised by talloc
> library. Allowing an object to cancel its own destruction is funny
> and really unexpected feature.
>
If you think hard about it, it actually makes a lot of sense to avoid
and prevents recursion, which can never be a good thing when freeing
resources.
Sure it's convenient. I just say it is unusual, at least for seasoned
C++ programmer.
>> Note that the patch still includes my old changes to avoid
freeing
>> ctx->gsh, they are *not* necessary, but I think they are good
>> hygiene anyway so I left them in.
>>
>>
> I think that these changes need to go to separate patch. Look for
> further discussion below.
>
Ok I split the patch in 2 parts.
0002-Avoid-freeing-sdap_handle-too-early.patch is mostly OK. I have only
one concern with it, see below.
0001-Better-handle-sdap_handle-memory-from-callers.patch, from my point
of view, should be dropped or integrated into later patch addressing
failover reconnect problem as:
- The patch itself does not solve failover reconnect problems (it
just adds a bit of robustness that is good, but is not sufficient)
- 0002-Avoid-freeing-sdap_handle-too-early.patch fixes LDAP backend
crash on its own and therefore failover reconnect problem could wait for
compete solution.
<...>
> 4. It's bad policy to destroy memory allocated and owned by other
> code (in sdap_handle_release):
> if (op == sh->ops) talloc_free(op);
> So I suggest to use early call of sdap_op destructor instead of
> deallocation (see my patch)
>
In this case although the memory is child of the original requests, the
semantic owner is the sdap_handle(), so it is eantirely appropriate to
free the operations if sdap_handle is being freed, for the simple
reason that the "technical" owner has no access to those objects and
they would be just hanging memory to their requests.
I still disagree with this approach. The memory must be deleted only by
owner. If it is not deleted after callback completes,
sdap_handle_release must assume that it is still needed for some
purpose. Destroying memory that is not owned is a sure way to problems.
Nevertheless in current sdap_handle usage, sdap_op is __always__ deleted
by owner. So it would make no major harm if you stay with your point of
view. It actually does not matter in current state of development.
Eugene