[Date Prev][Date Next]
[Chronological]
[Thread]
[Top]
Re: Proposed patch to support: draft-zeilenga-ldap-c-api-concurrency
Doug,
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
lconn_refcnt).
3) specific support to draft-zeilenga-ldap-c-api-concurrency, including
but not limited to the addition of ldap_dup(), ldap_destroy() and related
stuff.
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:
> LDAP_OPT_MATCHED_DN
> LDAP_OPT_DIAGNOSTIC_MESSAGE
> LDAP_OPT_RESULT_CODE
> 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
>