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

Reply via email to