a list of relatively quick comments, as I didn't go into too much detail
in the analysis of each bit of your work.
I think your patch contains at least three levels of modifications; they
all make sense separately and incrementally, so I think we should revise
1) "cosmetic" cleanup (like wrapping locks in specific macros); they do
not strictly relate to the support of
draft-zeilenga-ldap-c-api-concurrency (but they make sense, and I like
them); they should be considered first, regardless of the rest (I note
that according to the way you define LDAP_NEXT_MSGID when #ifdef
LDAP_R_COMPILE, you don't need to #ifdef its definition :)
2) a set of issues that fix the concurrent behavior of libldap relatively
irrespective of the support of draft-zeilenga-ldap-c-api-concurrency (see
for example the private discussion we had on fixing concurrent access to
3) specific support to draft-zeilenga-ldap-c-api-concurrency, including
but not limited to the addition of ldap_dup(), ldap_destroy() and related
As you may have understood, I'm especially interested in (2), because it
fixes a number of potential (or actual) issues in OpenLDAP code that uses
libldap concurrently, like the proxy backends. So far, that code used
libldap taking into account known weaknesses from a concurrency
standpoint. For example, proxy backends protect handlers until a bind
succeeds, to prevent the undesired simultaneous creation of default
connections. However, some issues may slip in (as I believe is the case
One thing I'd like to ask is: you introduce a few additional mutexes:
ldapoptions -> ldo_mutex
ldapcommon -> ldc_msgid_mutex, ldc_abandon_mutex
in addition of the already existing
ldap -> ld_conn_mutex, ld_req_mutex, ld_res_mutex
that move to ldap_common.
My concern is: can you guarantee that the occurrences of locking/unlocking
those additional mutexes, combined to the existing ones, do not result in
deadlocks? I mean: did you explicitly check all possible logical paths or
so, or take measures to avoid this possibility?
Another mostly "style" comment: I think you shouldn't be polluting
libldap's space with thread implementation specific details, like
#if !(defined(HAVE_THR) || defined(HAVE_PTHREADS))
and so. I understand PTHREAD_MUTEX_INITIALIZER is handy, but the whole
thing should remain confined in libldap_r specific files.
Also, but since it's a test tool it is not a priority, slapd-mtread.c
should use libldap_r and hide thread implementation details under
OpenLDAP's portable interface.