[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: Proposed patch to support: draft-zeilenga-ldap-c-api-concurrency


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
them separately.

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
of ITS#6574).

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?

Thanks, p.

>   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
> drafts.
> 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
>      ldap_get_lderror
> which returns all three ldap_get_option results:
> in a single API, and 1 new depreciated public API:
>     ldap_get_lderrno
> 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
> modifications to
> 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
> Oracle Corporation,
> my employer, to release this work under the following terms.
> The initial version of my proposed patch can be found here:
> http://cr.opensolaris.org/~djl/openldap-codereview
> 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.
> Doug Leavitt