On Thu, Apr 29, 2010 at 01:47:42PM -0400, 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.
> > 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.
> > The only changes really necessary are the ones to add the 2
> > booleans to sdap_handle in sdap.h and the changes to
> > sdap_handle_destructor() and sdap_handle_release() in sdap_async.c
> >
> > These changes are much less invasive than the patch you propose and
> > it looks to me they provide for the same outcome.
> >
> > Of course comments are welcome.
> >
> I think that several more or less cosmetic additions could be of some
> value, so I have created my own version of the patch. I know that
> this looks more or less as not-invented-here syndrome, so you could
> discard the patch altogether without looking into it. I have attached
> it merely to illustrate the following points that seem valuable to me:
Ok I took some advice, but my patch remains mostly unchanged.
> 1. Better names for sh->locked and sh->release respectively could be
> sh->being_disconnected and sh->orphan. Some additional comments would
> also be of some help to future maintainers.
I agree the names where not the best I could think, but I prefer to
give names that represent the function of the variable rather than what
is supposedly going to happen when the variable is true.
I renamed lock -> destructor_lock, so it is clear what it is locking.
I also renamed release -> release_memory, orphan doesn't fit as memory
is not really orphan of anything, it is still child of whatever parent
until freed, but we want to signal that memory needs be released once
we are done.
> 2. A new method sdap_handle_disconnect should be used to forcibly
> disconnect sdap_handle. Setting sh->connected = false (as in full
> version of patch) is no good as it leaves the LDAP connection open
> for indeterminate period of time and violates object encapsulation.
I' leave this for a following patch if necessary.
> 3. sdap_handle_release should unregister sh->connb from LDAP prior to
> deleting it. Although NULL pointer check is done LDAP callback, it's
> good policy to remove callbacks when no longer needed:
> ldap_get_option(sh->ldap, LDAP_OPT_CONNECT_CB, sh->conncb);
This is now taken care of automatically with latest Sumit patches,
thanks for pointing it out.
> 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 propose the attached patches for inclusion in master.
I'd like to divert any modification that is not an intrinsic bug in the
patches to later patches and a separate thread so we can move on :)
Simo.
--
Simo Sorce * Red Hat, Inc * New York
ACK to both.
I have seen Eugene comments on the patches and do not want to argure
that these patches make the code only cleaner and more robust. But I
think these improvements are still worth to be added to the current code
base and some of them might get lost if we postponed them to a later
patch.
bye,
Sumit