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.
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.
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:
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.
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.
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);
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)
//-------------------------------------
So now we are ready to continue with fixing failover reconnect and
GSSAPI authentication in LDAP and IPA providers. From my point of view
at least the following problems needs to be addressed by final solution:
1. When two (or more) BE requests are executed in parallel and there is
no cached connection, only one LDAP connection should be established. In
current implementation 2 connections will be established and the first
one killed failing the operation that connected first.
2. When OFFLINE state is detected during request execution (there were
cached connection, but all failover servers failed to connect during
request execution), the backend must return DP_ERR_OFFLINE. It currently
returns DP_ERR_FATAL with EIO error. Next request completes with
DP_ERR_OFFLINE. So there is a big inconsistency in behaviour.
3. It is essential to close LDAP connection before GSSAPI ticket is
expired as closing connection with already expired ticket still writes a
message in message log.
4. The about-to-expire connection should be closed gracefully: all
requests already in progress and using the connection should be
completed, new requests should establish and use new connection.
5. ipa_access backend should also use failover retries.
6. I think it is essential to reduce amount of copy-paste code handling
LDAP connect/reconnect code. My strong opinion is that a special
mechanism for handling LDAP connect/retry logic is required.
Eugene