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? - 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 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. 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. 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 cc7ee7f7e8d934c470a72f9dd1735c0de54a62b9 Mon Sep 17 00:00:00 2001 > From: Stephen Gallagher <sgall...@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 | 2 ++ > src/providers/ldap/sdap_async_private.h | 1 + > src/providers/ldap/sdap_fd_events.c | 16 ++++++++++++++++ > 3 files changed, 19 insertions(+), 0 deletions(-) > > diff --git a/src/providers/ldap/ldap_common.c > b/src/providers/ldap/ldap_common.c > index > 0257ef63850982fcf8334c09fb6f278b735a9ad8..8af311d1ec213855e17f1e5745a6c1aff78ad3f0 > 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" > > @@ -362,6 +363,7 @@ void sdap_mark_offline(struct sdap_id_ctx *ctx) > /* 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; > + remove_ldap_connection_callbacks(ctx->gsh); > } > > 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@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel