On (13/12/13 17:44), Petr Spacek wrote:
>On 12.11.2013 16:13, Petr Spacek wrote:
>>On 5.11.2013 12:29, Tomas Hozza wrote:
>>>----- Original Message -----
>>>>Hello,
>>>>
>>>>Improve performance of initial LDAP synchronization.
>>>>
>>>>Changes are not journaled and SOA serial is not incremented during initial
>>>>LDAP synchronization.
>>>>
>>>>This eliminates unnecessary synchronous writes to journal and also
>>>>unnecessary SOA serial writes to LDAP.
>>>>
>>>>See commit messages and comments in syncrepl.c for all the gory details.
>>>
>>>
>>>ACK.
>>>
>>>Patches look good. AXFR and IXFR works as expected. Also BIND starts up much
>>>faster with these patches. Good job... :)
>>>
>>>Regards,
>>>
>>>Tomas
>>
>>Hmm, further testing revealed that patch 203 changed behavior little bit:
>>Zones were loaded from LDAP correctly, but the SOA serial wasn't changed at
>>all. As a result, zone transfers return inconsistent results if the data in
>>LDAP are changed when BIND was not running.
>>
>>Patch 203-v2 imitates the old behavior from bind-dyndb-ldap 3.x: Zone serial
>>is bumped *once* for each zone, so any changed in LDAP will be transferred
>>correctly (with new serial).
>
>Patch 202 v2 was rebased and fixes reconnection to LDAP and solves
>deadlock caused by too eager locking.
>
>Patch 203 v3 was rebased and fixes reconnection to LDAP.
>
>These patches should go to master branch.
>
>--
>Petr^2 Spacek
>
When I was testing upcoming bind-dyndb-ldap 4.0 release,
There was an interesting warning from clang static analyser.
I thought it was a false passitive, but it isn't.
Patch is attached
>From b73e345393d55fe411875d52e6fe4c98e1de8c04 Mon Sep 17 00:00:00 2001
>From: Petr Spacek <pspa...@redhat.com>
>Date: Mon, 9 Dec 2013 11:11:10 +0100
>Subject: [PATCH] Detect end of initial LDAP synchronization phase.
>
>LDAP intermediate message with refreshDone = TRUE is translated to
>sync_barrier_wait() call. This call sends 'barrier event' to all tasks
>involved in syncrepl event processing. The call will return when all tasks
>have processed all events enqueued before the call.
>
>Effectively, all events produced by initial LDAP synchronization
>are processed first. Current state of synchronization is available via
>sync_state_get() call.
>
>See comments in syncrepl.c for all the gory details.
>
>Signed-off-by: Petr Spacek <pspa...@redhat.com>
>---
> src/Makefile.am | 2 +
> src/ldap_helper.c | 67 +++++++++--
> src/ldap_helper.h | 2 +
> src/syncrepl.c | 351 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> src/syncrepl.h | 63 ++++++++++
> 5 files changed, 473 insertions(+), 12 deletions(-)
> create mode 100644 src/syncrepl.c
> create mode 100644 src/syncrepl.h
>
<snip>
>+/**
>+ * Initialize synchronization context.
>+ *
>+ * @param[in] task Task used for first synchronization events.
>+ * Typically the ldap_inst->task.
>+ * @param[out] sctxp The new synchronization context.
>+ *
>+ * @post state == sync_init
>+ * @post task_cnt == 1
>+ * @post tasks list contains the task
>+ */
>+isc_result_t
>+sync_ctx_init(isc_mem_t *mctx, isc_task_t *task, sync_ctx_t **sctxp) {
>+ isc_result_t result;
>+ sync_ctx_t *sctx = NULL;
>+ isc_boolean_t lock_ready = ISC_FALSE;
>+ isc_boolean_t cond_ready = ISC_FALSE;
>+ isc_boolean_t refcount_ready = ISC_FALSE;
>+
>+ REQUIRE(sctxp != NULL && *sctxp == NULL);
^^^^^^^^^^^^^^
*sctxp is NULL
>+ REQUIRE(ISCAPI_TASK_VALID(task));
>+
>+ CHECKED_MEM_GET_PTR(mctx, sctx);
>+ ZERO_PTR(sctx);
>+ isc_mem_attach(mctx, &sctx->mctx);
>+
>+ CHECK(isc_mutex_init(&sctx->mutex));
>+ lock_ready = ISC_TRUE;
>+ CHECK(isc_condition_init(&sctx->cond));
>+ cond_ready = ISC_TRUE;
>+
>+ /* refcount includes ldap_inst->task implicitly */
>+ CHECK(isc_refcount_init(&sctx->task_cnt, 0));
>+ refcount_ready = ISC_TRUE;
>+
>+ ISC_LIST_INIT(sctx->tasks);
>+
>+ sctx->state = sync_init;
>+ CHECK(sync_task_add(sctx, task));
>+
>+ *sctxp = sctx;
^^^^^^
value to *sctxp is asigned only on this line.
>+ return ISC_R_SUCCESS;
>+
>+cleanup:
*sctxp will be NULL in cleanup section
>+ if (lock_ready == ISC_TRUE)
>+ isc_mutex_destroy(&(*sctxp)->mutex);
&(NULL)->mutex
It does not look like a good idea :-)
>+ if (cond_ready == ISC_TRUE)
>+ isc_condition_init(&(*sctxp)->cond);
>+ if (refcount_ready == ISC_TRUE)
>+ isc_refcount_destroy(&(*sctxp)->task_cnt);
>+ MEM_PUT_AND_DETACH(*sctxp);
>+ return result;
>+}
>+
LS
>From dcf99122eb33b63799be91d3c13a9967420880c3 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lsleb...@redhat.com>
Date: Fri, 21 Feb 2014 19:18:28 +0100
Subject: [PATCH] Fix potential dereference of NULL pointer in sync_ctx_init
Value is asigned to the output argument sync_ctx_t **sctxp
before returning ISC_R_SUCCESS. Thus we should use local variable sctx
in cleanup section.
---
src/syncrepl.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/syncrepl.c b/src/syncrepl.c
index
2a6d159fe16fffbf928198f9240cd8b1fcd41005..c6b326bab9733f7cb5065381d26e999ddbb570db
100644
--- a/src/syncrepl.c
+++ b/src/syncrepl.c
@@ -215,12 +215,12 @@ sync_ctx_init(isc_mem_t *mctx, isc_task_t *task,
sync_ctx_t **sctxp) {
cleanup:
if (lock_ready == ISC_TRUE)
- isc_mutex_destroy(&(*sctxp)->mutex);
+ isc_mutex_destroy(&sctx->mutex);
if (cond_ready == ISC_TRUE)
- isc_condition_init(&(*sctxp)->cond);
+ isc_condition_init(&sctx->cond);
if (refcount_ready == ISC_TRUE)
- isc_refcount_destroy(&(*sctxp)->task_cnt);
- MEM_PUT_AND_DETACH(*sctxp);
+ isc_refcount_destroy(&sctx->task_cnt);
+ MEM_PUT_AND_DETACH(sctx);
return result;
}
--
1.8.5.3
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
