On Wed, Aug 15, 2012 at 01:20:08PM +0200, Petr Spacek wrote:
> Hello,
> 
> this two patches solves upstream ticket
> https://fedorahosted.org/bind-dyndb-ldap/ticket/71
> "Log successful reconnect"
> 
> Patch 51:
>     Adds log_info(): logging facility with log level INFO.

Ack.

> Patch 52:
>     Logs successful reconnection to LDAP server.
> 
>     LDAP connection error handling was modified:
>     Errors are handled exclusively by handle_connection_error() now.
> 
>     Direct calls to ldap_connect() and ldap_reconnect() should be avoided.

Nack, please check my comments below.

Regards, Adam

> From 15286f0793d3666845e6b03b565d49f135b115ff Mon Sep 17 00:00:00 2001
> From: Petr Spacek <pspa...@redhat.com>
> Date: Wed, 15 Aug 2012 12:05:56 +0200
> Subject: [PATCH] Add log_info(): logging facility with log level INFO.
> 
> Signed-off-by: Petr Spacek <pspa...@redhat.com>
> ---
>  src/log.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/log.h b/src/log.h
> index 
> 898639be144dbf6049a1440493c3358e01a5c2dd..9062a4e80786b5bab806d6c9ceba1d1a9917a3e2
>  100644
> --- a/src/log.h
> +++ b/src/log.h
> @@ -43,6 +43,9 @@
>  #define log_error(format, ...)       \
>       log_write(GET_LOG_LEVEL(ISC_LOG_ERROR), format, ##__VA_ARGS__)
>  
> +#define log_info(format, ...)        \
> +     log_write(GET_LOG_LEVEL(ISC_LOG_INFO), format, ##__VA_ARGS__)
> +
>  #define log_debug(level, format, ...)        \
>       log_write(GET_LOG_LEVEL(level), format, ##__VA_ARGS__)
>  
> -- 
> 1.7.11.2
> 

> From 06f03d8b602656dc9581abc675c943d6fa6a6db2 Mon Sep 17 00:00:00 2001
> From: Petr Spacek <pspa...@redhat.com>
> Date: Wed, 15 Aug 2012 12:57:32 +0200
> Subject: [PATCH] Log successful reconnection to LDAP server.
> 
> LDAP connection error handling was modified:
> Errors are handled exclusively by handle_connection_error() now.
> 
> Direct calls to ldap_connect() and ldap_reconnect() should be avoided.
> 
> https://fedorahosted.org/bind-dyndb-ldap/ticket/71
> 
> Signed-off-by: Petr Spacek <pspa...@redhat.com>
> ---
>  src/ldap_helper.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/src/ldap_helper.c b/src/ldap_helper.c
> index 
> cc7003a6cdcd2d27404fec936623ed8a3e8fa7f8..798aeadfef27d7071a1dd4133b7f08a21918ef78
>  100644
> --- a/src/ldap_helper.c
> +++ b/src/ldap_helper.c
> @@ -1734,7 +1734,7 @@ ldap_query(ldap_instance_t *ldap_inst, 
> ldap_connection_t *ldap_conn,
>                * successful
>                * TODO: handle this case inside ldap_pool_getconnection()?
>                */
> -             CHECK(ldap_connect(ldap_inst, ldap_conn, ISC_FALSE));
> +             CHECK(handle_connection_error(ldap_inst, ldap_conn, ISC_FALSE));
>       }
>  
>  retry:
> @@ -1903,14 +1903,16 @@ ldap_connect(ldap_instance_t *ldap_inst, 
> ldap_connection_t *ldap_conn,
>       int ret;
>       int version;
>       struct timeval timeout;
> +     isc_result_t result;

I would recommend to be consistent here and use "isc_result_t result = 
ISC_R_FAILURE"

> +     REQUIRE(ldap_inst != NULL);
>       REQUIRE(ldap_conn != NULL);
>  
>       ret = ldap_initialize(&ld, str_buf(ldap_inst->uri));
>       if (ret != LDAP_SUCCESS) {
>               log_error("LDAP initialization failed: %s",
>                         ldap_err2string(ret));
> -             goto cleanup;
> +             CHECK(ISC_R_FAILURE);

Please use goto here, as done in rest of code.

>       }
>  
>       version = LDAP_VERSION3;
> @@ -1932,21 +1934,22 @@ ldap_connect(ldap_instance_t *ldap_inst, 
> ldap_connection_t *ldap_conn,
>       if (ldap_conn->handle != NULL)
>               ldap_unbind_ext_s(ldap_conn->handle, NULL, NULL);
>       ldap_conn->handle = ld;
> +     ld = NULL; /* prevent double-unbind from ldap_reconnect() and cleanup: 
> */
>  
> -     return ldap_reconnect(ldap_inst, ldap_conn, force);
> +     CHECK(ldap_reconnect(ldap_inst, ldap_conn, force));
> +     return result;
>  
>  cleanup:
> -
>       if (ld != NULL)
>               ldap_unbind_ext_s(ld, NULL, NULL);
>       
>       /* Make sure handle is NULL. */
>       if (ldap_conn->handle != NULL) {
>               ldap_unbind_ext_s(ldap_conn->handle, NULL, NULL);
>               ldap_conn->handle = NULL;
>       }
>  
> -     return ISC_R_FAILURE;
> +     return result;
>  }
>  
>  static isc_result_t
> @@ -2064,12 +2067,18 @@ handle_connection_error(ldap_instance_t *ldap_inst, 
> ldap_connection_t *ldap_conn
>  {
>       int ret;
>       int err_code;
> +     isc_result_t result = ISC_R_FAILURE;
>  
> -     ret = ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE,
> -                           (void *)&err_code);
> +     REQUIRE(ldap_conn != NULL);

...

> -     if (ret != LDAP_OPT_SUCCESS) {
> -             log_error("handle_connection_error failed to obtain ldap error 
> code");
> +     if (ldap_conn->handle != NULL) {
> +             ret = ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE,
> +                                       (void *)&err_code);
> +             if (ret != LDAP_OPT_SUCCESS) {
> +                     log_error("handle_connection_error failed to obtain 
> ldap error code");
> +             }
> +     }
> +     if (ldap_conn->handle == NULL || ret != LDAP_OPT_SUCCESS) {
>               goto reconnect;
>       }

I think this is more readable:

        if (ldap_conn->handle == NULL)
                goto reconnect;

        ret = ldap_get_option(..);
        if (ret != LDAP_OPT_SUCCESS) {
                log_error
                goto reconnect;
        }

isn't it?

>  
> @@ -2090,11 +2099,13 @@ handle_connection_error(ldap_instance_t *ldap_inst, 
> ldap_connection_t *ldap_conn
>  reconnect:
>               if (ldap_conn->tries == 0)
>                       log_error("connection to the LDAP server was lost");
> -             return ldap_connect(ldap_inst, ldap_conn, force);
> -             
> +             result = ldap_connect(ldap_inst, ldap_conn, force);
> +             if (result == ISC_R_SUCCESS)
> +                     log_info("successfully reconnected to LDAP server");
> +             break;
>       }
>  
> -     return ISC_R_FAILURE;
> +     return result;
>  }
>  
>  /* FIXME: Handle the case where the LDAP handle is NULL -> try to reconnect. 
> */
> @@ -3476,7 +3487,7 @@ ldap_psearch_watcher(isc_threadarg_t arg)
>                         "Next try in %ds", inst->reconnect_interval);
>               if (!sane_sleep(inst, inst->reconnect_interval))
>                       goto cleanup;
> -             ldap_connect(inst, conn, ISC_TRUE);
> +             handle_connection_error(inst, conn, ISC_TRUE);
>       }
>  
>       CHECK(ldap_query_create(conn->mctx, &ldap_qresult));
> -- 
> 1.7.11.2
> 


-- 
Adam Tkac, Red Hat, Inc.

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to