On 06/08/2010 07:09 AM, Stephen Gallagher wrote: >On 06/08/2010 03:00 AM, Sumit Bose wrote: >>On Mon, Jun 07, 2010 at 04:02:26PM -0400, Stephen Gallagher wrote: >>>Sumit, I'm requesting that you perform this review, as you are most >>>familiar with this bit of code. >>> >>> >>>Under certain circumstances, the openldap libraries will continue >>>internally trying to reconnect to a connection lost (as during a >>>cable-pull test). We need to drop the reconnection callbacks when >>>marking the backend offline in order to guarantee that they are >>>not called with an invalid sdap_handle. >>> >>>This addresses https://fedorahosted.org/sssd/ticket/494 >> >>Unfortunately I was not able to reproduce #494 here, but if this patch >>works for you I'm fine with it. I only have two minor nitpicks: >> >>- why don't you use talloc_zfree directly? > >Because for some reason I haven't yet worked out, talloc_zfree() was >giving me a syntax error. In the interest of having a working patch, I >just bypassed it. > Ok, I realized what was wrong. talloc_zfree() does not return a value, so it can't be on the right side of an equals. :) However, I really do want to check the return value here, since one of the attached destructors can legitimately return -1 here. So I'm leaving this unchanged.

>>- remove_ldap_connection_callbacks() has a return value, so you should >> either check it and print a debug message in the case of an error or >> define it as void >> > >Thanks, I'll fix that Fixed. It now prints a debug message and adds a comment that even on failure we're going to attempt to proceed. . > >>This problem might have its origin in 'Better handle sdap_handle memory >>from callers' where we remove a number of talloc_zfree(sdap_ctx->gsh) >>calls. > >Yeah, that seems likely >> >>I would prefer to ACK this patch only for the 1.2 branch to solve #494 >>for this branch. Because it is planned to improve the handling of the >>global LDAP connetion for 1.3 I hope a cleaner solution can be found for >>master. >> > >Would you mind acking it also for master (once this passes review) until >we get to that fix? I don't want to leave a serious known bug in the >tree for an indeterminate amount of time. >

-- Stephen Gallagher RHCE 804006346421761 Delivering value year after year. Red Hat ranks #1 in value among software vendors. http://www.redhat.com/promo/vendor/

From 9b5821d52667e83462b6ec5a72e43a31350d18ed Mon Sep 17 00:00:00 2001 From: Stephen Gallagher <sgallagh(a)redhat.com&gt; Date: Mon, 7 Jun 2010 15:59:26 -0400 Subject: [PATCH] Disable connection callbacks when going online Under certain circumstances, the openldap libraries will continue internally trying to reconnect to a connection lost (as during a cable-pull test). We need to drop the reconnection callbacks when marking the backend offline in order to guarantee that they are not called with an invalid sdap_handle. --- src/providers/ldap/ldap_common.c | 10 ++++++++++ src/providers/ldap/sdap_async_private.h | 1 + src/providers/ldap/sdap_fd_events.c | 16 ++++++++++++++++ 3 files changed, 27 insertions(+), 0 deletions(-) diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c index 0257ef63850982fcf8334c09fb6f278b735a9ad8..78bcac2dbbd5b5ab469dc7cea49e005045f06ce1 100644 --- a/src/providers/ldap/ldap_common.c +++ b/src/providers/ldap/ldap_common.c @@ -24,6 +24,7 @@ #include "providers/ldap/ldap_common.h" #include "providers/fail_over.h" +#include "providers/ldap/sdap_async_private.h" #include "util/sss_krb5.h" @@ -358,10 +359,19 @@ bool sdap_connected(struct sdap_id_ctx *ctx) void sdap_mark_offline(struct sdap_id_ctx *ctx) { + int ret; + if (ctx->gsh) { /* make sure we mark the connection as gone when we go offline so that * we do not try to reuse a bad connection by mistale later */ ctx->gsh->connected = false; + ret = remove_ldap_connection_callbacks(ctx->gsh); + if (ret != EOK) { + DEBUG(1, ("Could not clear ldap connection callbacks\n")); + /* Not really anything we can do about this, so proceed + * and hope for the best. + */ + } } be_mark_offline(ctx->be); diff --git a/src/providers/ldap/sdap_async_private.h b/src/providers/ldap/sdap_async_private.h index 727cee2e0e87e5efa0340d3a94d1589022f6676c..ac91a0105f517116d0012711caf5e271038750d6 100644 --- a/src/providers/ldap/sdap_async_private.h +++ b/src/providers/ldap/sdap_async_private.h @@ -33,6 +33,7 @@ void sdap_ldap_result(struct tevent_context *ev, struct tevent_fd *fde, int setup_ldap_connection_callbacks(struct sdap_handle *sh, struct tevent_context *ev); +int remove_ldap_connection_callbacks(struct sdap_handle *sh); int get_fd_from_ldap(LDAP *ldap, int *fd); diff --git a/src/providers/ldap/sdap_fd_events.c b/src/providers/ldap/sdap_fd_events.c index 3278296308fcb1a745dbd4cb89a4cc0c05aa065b..f989c2249900a75bf2c56fb2ded649ae877cca78 100644 --- a/src/providers/ldap/sdap_fd_events.c +++ b/src/providers/ldap/sdap_fd_events.c @@ -47,7 +47,23 @@ int get_fd_from_ldap(LDAP *ldap, int *fd) return EOK; } +int remove_ldap_connection_callbacks(struct sdap_handle *sh) +{ #ifdef HAVE_LDAP_CONNCB + int ret; + + ret = talloc_free(sh->sdap_fd_events->conncb); + if (ret != 0) { + return EIO; + } + + sh->sdap_fd_events->conncb = NULL; +#endif + return EOK; +} + +#ifdef HAVE_LDAP_CONNCB + static int remove_connection_callback(TALLOC_CTX *mem_ctx) { int lret; -- 1.7.0.1

_______________________________________________ sssd-devel mailing list sssd-devel(a)lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel