On Tue, Jun 08, 2010 at 08:03:45AM -0400, Stephen Gallagher wrote:
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.
ok
>>- 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.
>
ok, fair enough.
ACK for 1.2 and master
bye,
Sumit
--
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>
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