[Freeipa-devel] [PATCH 0051-0052] Log successful reconnection to LDAP server
Petr Spacek
pspacek at redhat.com
Wed Sep 5 13:53:36 UTC 2012
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 <pspacek at 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 <pspacek at 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
>>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bind-dyndb-ldap-pspacek-0052-2-Log-successful-reconnection-to-LDAP-server.patch
Type: text/x-patch
Size: 3620 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120905/5c7eb489/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bind-dyndb-ldap-pspacek-0052a-Add-CLEANUP_WITH-macro-for-unconditional-result-code.patch
Type: text/x-patch
Size: 804 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120905/5c7eb489/attachment-0001.bin>
More information about the Freeipa-devel
mailing list