On 09/05/2012 01:29 PM, Adam Tkac wrote:
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.
Thanks for your comments! Corrected patches are attached + I replied in-line.
Regards, Adam
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"
My fault, corrected. It is definitely better to initialize it to avoid
surprises in a future.
+ 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.
My intent was to replace
result = ISC_R_...;
goto cleanup;
with
CHECK(ISC_R_...);
to make it more readable. I don't like "alone" goto, because "result" value
can change after code changes in future.
Well, CHECK() wasn't good choice for this. I introduced CLEANUP_WITH() macro
to distinguish jump with constant result from CHECK(function_call()) cases.
Attached patch 0052a adds CLEANUP_WITH() macro.
This distinguishing should allow to realize failure injection to CHECK()
macros (in far future :-).
}
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?
Yes, it is. You saw my unsuccessful attempt to concentrate gotos to one place.
I replaced it with you proposal (actually the original code :-)).
Petr^2 Spacek
@@ -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
From faf89f0c3a2f75b94f9e80e1a923e2dd1e75376f Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Wed, 5 Sep 2012 15:30:32 +0200
Subject: [PATCH] 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.
Signed-off-by: Petr Spacek <pspa...@redhat.com>
---
src/ldap_helper.c | 32 +++++++++++++++++++++-----------
1 file changed, 21 insertions(+), 11 deletions(-)
diff --git a/src/ldap_helper.c b/src/ldap_helper.c
index f83dad1fdf29c69338afdf4478c4bb400f7936f4..a01e67ebef6c2b40c099613c0081cc5bb396b21d 100644
--- a/src/ldap_helper.c
+++ b/src/ldap_helper.c
@@ -1725,7 +1725,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:
@@ -1914,14 +1914,16 @@ ldap_connect(ldap_instance_t *ldap_inst, ldap_connection_t *ldap_conn,
int ret;
int version;
struct timeval timeout;
+ 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;
+ CLEANUP_WITH(ISC_R_FAILURE);
}
version = LDAP_VERSION3;
@@ -1943,21 +1945,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
@@ -2075,10 +2078,15 @@ 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;
+
+ REQUIRE(ldap_conn != NULL);
+
+ if (ldap_conn->handle == NULL)
+ goto reconnect;
ret = ldap_get_option(ldap_conn->handle, LDAP_OPT_RESULT_CODE,
- (void *)&err_code);
-
+ (void *)&err_code);
if (ret != LDAP_OPT_SUCCESS) {
log_error("handle_connection_error failed to obtain ldap error code");
goto reconnect;
@@ -2101,11 +2109,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;
}
static isc_result_t
@@ -3486,7 +3496,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.4
From 7b85cf3b08467e15289f20ee96e18d33e25635e0 Mon Sep 17 00:00:00 2001
From: Petr Spacek <pspa...@redhat.com>
Date: Wed, 5 Sep 2012 15:24:10 +0200
Subject: [PATCH] Add CLEANUP_WITH() macro for unconditional result code
assignment and jump to cleanup section.
Signed-off-by: Petr Spacek <pspa...@redhat.com>
---
src/util.h | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/util.h b/src/util.h
index 9e48ae88ba3107efccd544f082e49652e7622940..2b8f10e59ddacdb1d0e49cf0de3e9ab8a3786090 100644
--- a/src/util.h
+++ b/src/util.h
@@ -21,6 +21,12 @@
#ifndef _LD_UTIL_H_
#define _LD_UTIL_H_
+#define CLEANUP_WITH(result_code) \
+ do { \
+ result = (result_code); \
+ goto cleanup; \
+ } while(0)
+
#define CHECK(op) \
do { \
result = (op); \
--
1.7.11.4
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
