[SSSD] [PATCH] Disable connection callbacks when going online

Sumit Bose sbose at redhat.com
Tue Jun 8 07:00:26 UTC 2010


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 <sgallagh at 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
> 




More information about the sssd-devel mailing list