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