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 >
