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?
I would like to propose a patch to the OpenLDAP
C APIs to support the operation thread safe features
described in the draft-zeilenga-ldap-c-api-concurrency
The enhancements add upwardly compatible interfaces
and should have no behavioral regressions w.r.t. any
existing libldap interfaces.
The new concurrency APIs include:
ldap_dup and ldap_destroy
They are described in more detail in the concurrency draft
and in the applicable man paged. The related feature
options are also provided.
Additionally I would like to propose 1 new public error API
which returns all three ldap_get_option results:
in a single API, and 1 new depreciated public API:
which performs almost the same function as ldap_get_error
but does so in a mt-unsafe manner, but that is compatible with
Mozilla's libldap API of the same name.
The point of the two new error APIs is to provide a transition
path from Mozilla libldap users that still use ldap_get_lderrno to either
a similar API that obeys OpenLDAP like practices (ldap_get_error)
or as a transition until any existing code can be converted away from
ldap_get_lderrno to just using ldap_get_options as needed.
The patch includes a new test case to test the new functionality
and man pages.
The new test case needs additional work for some
of the threading models. Any input here is requested, since
I do not have testing resources for all the threading combination
and do not have the ability to test the Windows model.
I am looking forward to any and all comments.
I plan to submit a formal ITS with a finalized version of changes once
any and all issues are thrashed out and resolved to a level that
they are acceptable for integration.
Just for the record:
This patch file is derived from OpenLDAP Software. All of the
OpenLDAP Software represented in the following patch(es) were developed
by Oracle Corporation.Oracle Corporation has not assigned rights and/or
interest in this work to any party. I, Douglas Leavitt am authorized by
my employer, to release this work under the following terms.
The initial version of my proposed patch can be found here:
This includes a
diff -U of the full patch (/diffU)
a webrev diff (/webrev) for on-line review
and a tar ball image of the OpenLDAP HEAD as of 8/13/10
with the current patch applied.
Thank you for you consideration, and I look forward
to any and all comments.