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
Doug,
I note as a general rule, which I will follow in this case, I don't review patches authors would like included in OpenLDAP Software until they have been formally submitted for review.
Regards, Kurt
Kurt Zeilenga wrote:
Doug,
I note as a general rule, which I will follow in this case, I don't review
patches authors would like included in OpenLDAP Software until they have been formally submitted for review.
While the ITS submission provides for a review of what was done, my purpose in directing the conversation here is to discuss why. It's not clear to me that this old draft is still viable or desirable. As we've talked about many times before, the C API is a huge mess and we need to toss the current one out and go back to the drawing board, preferably to come up with a new API that doesn't meld operational behavior (e.g. referral chasing) with the low level protocol implementation, and one that is largely free-threaded.
On 08/18/10 06:34 PM, Howard Chu wrote:
Kurt Zeilenga wrote:
Doug,
I note as a general rule, which I will follow in this case, I don't review
patches authors would like included in OpenLDAP Software until they have been formally submitted for review.
While the ITS submission provides for a review of what was done, my purpose in directing the conversation here is to discuss why. It's not clear to me that this old draft is still viable or desirable. As
Our prime motivation for integrating operation safe enhancements into OpenLDAP libldap is because we want to move off Mozilla libldap, and specifically off an old Solaris version of Mozilla libldap that has Solaris specific tweaks. Our end goal is to deliver generic OpenLDAP releases into Solaris, where the OpenLDAP libs/headers replace the existing core Solaris libldap libs/headers and we deliver the same level of compatibility as other distributions.
That said, Solaris naming services currently requires operation safe libldap operations as part of our LDAP naming support. We routinely multiplex requests from multiple threads over a single connection, or a small pool of connections using the known Mozilla libldap threading behaviors.
As I interpret draft-zeilenga-ldap-c-api-concurrency, the proposal introduces the ldap_dup and ldap_destroy APIs that both enable the operation safe enhancements we desire, and keep the existing OpenLDAP libldap behaviors 100% backwards compatible with previous releases.
So, I see the draft as a relevant and viable solution, that has already been implemented once AFAIK (Novell), and provides a valuable feature we (Solaris) strongly desire. I believe that other applications that desire to manage shared connections in a multi-threaded environment will also want to use this functionality.
Doug.
On Aug 18, 2010, at 4:34 PM, Howard Chu wrote:
my purpose in directing the conversation here is to discuss why. It's not clear to me that this old draft is still viable or desirable. As we've talked about many times before, the C API is a huge mess and we need to toss the current one out and go back to the drawing board, preferably to come up with a new API that doesn't meld operational behavior (e.g. referral chasing) with the low level protocol implementation, and one that is largely free-threaded.
At one time, I must have through the draft's suggested changes to the LDAPext-proposed U-Mich-based LDAP C API where a good idea. Lots of time has passed since that time.
Taking a step back, as I'ved noted many times, I would support introduction of a new LDAP C API which focused on encoding/decoding of LDAP constructions and not I/O management and not higher level handling such as managing distributed contexts. Such a API could and, I certainly would argue, should be be thread-free. Given this, one could use this to offer higher level APIs. I would prefer to, in the long run, dump the U-Mich APIs as I think it's a dead-end (though I think that dead-end might be many, many miles down the road).
I don't object to continued work on the OpenLDAP LDAP C API. Bug fixes (Ando commented that some of the Doug's changes can be viewed as Bug fixes) are certainly not objectionable.
I also don't object to introduction of additional capability so long as that introduction doesn't "break" applications specifically designed to use prior (within reason) versions of the OpenLDAP LDAP C API upon upgrade.
I am generally opposed to adding additional APIs just to additional APIs which offer zero new capability and am generally opposed to adding new deprecated APIs. Also, I also don't generally accept the argument that introduction of interfaces found in other LDAP C APIs 'eases' porting of LDAP applications. But let's look at ldap_get_lderrno() specifically.
Today there are multiple flavors of the ldap_get_lderrno() API. I found two different non-OpenLDAP LDAP C APIs to include an ldap_get_lderrno() interface with the signature: int ldap_get_lderrno(LDAP *ld, char **dn, char **errmsg);
but with incompatible semantics. One required the caller to free the strings returned in dn and errmsg. One didn't, as the dn and errmsg were simply set to point to the strings held in the LDAP structure.
The expected problems introduced are differ depending on whether OpenLDAP ldap_get_lderrno(), it should be clear that problems are introduced. Simply put, despite the introduction of ldap_get_lderrno(), application developers need to be careful in porting to ensure that OpenLDAP offers compatible semantics to what they offer. And that likely requires reading, at a minimum, the documentation.
I rather developers simply port their code to use ldap_get_option(3) interface here, which because of inclusion in the LDAPEXT LDAP C API drafts (even through expired), are far more consistently implemented than routines such as ldap_get_lderrno().
I also oppose adding ldap_get_lderror().
Beyond this, I haven't reviewed the patch in detail. I think I'll leave that more current and more active in the development of OpenLDAP Software.
-- Kurt
I understand w.r.t. the depreciated and additional error functions. They are not specifically required for anything we are doing, I saw them as convenience APIs. I will remove them as part of my next round code changes.
I also wasn't aware that there were multiple flavors of the same API signature. I agree that adding another instance would only work to confuse the situation even more.
Doug.
On 08/22/10 01:53 PM, Kurt Zeilenga wrote:
On Aug 18, 2010, at 4:34 PM, Howard Chu wrote:
my purpose in directing the conversation here is to discuss why. It's not clear to me that this old draft is still viable or desirable. As we've talked about many times before, the C API is a huge mess and we need to toss the current one out and go back to the drawing board, preferably to come up with a new API that doesn't meld operational behavior (e.g. referral chasing) with the low level protocol implementation, and one that is largely free-threaded.
At one time, I must have through the draft's suggested changes to the LDAPext-proposed U-Mich-based LDAP C API where a good idea. Lots of time has passed since that time.
Taking a step back, as I'ved noted many times, I would support introduction of a new LDAP C API which focused on encoding/decoding of LDAP constructions and not I/O management and not higher level handling such as managing distributed contexts. Such a API could and, I certainly would argue, should be be thread-free. Given this, one could use this to offer higher level APIs. I would prefer to, in the long run, dump the U-Mich APIs as I think it's a dead-end (though I think that dead-end might be many, many miles down the road).
I don't object to continued work on the OpenLDAP LDAP C API. Bug fixes (Ando commented that some of the Doug's changes can be viewed as Bug fixes) are certainly not objectionable.
I also don't object to introduction of additional capability so long as that introduction doesn't "break" applications specifically designed to use prior (within reason) versions of the OpenLDAP LDAP C API upon upgrade.
I am generally opposed to adding additional APIs just to additional APIs which offer zero new capability and am generally opposed to adding new deprecated APIs. Also, I also don't generally accept the argument that introduction of interfaces found in other LDAP C APIs 'eases' porting of LDAP applications. But let's look at ldap_get_lderrno() specifically.
Today there are multiple flavors of the ldap_get_lderrno() API. I found two different non-OpenLDAP LDAP C APIs to include an ldap_get_lderrno() interface with the signature: int ldap_get_lderrno(LDAP *ld, char **dn, char **errmsg);
but with incompatible semantics. One required the caller to free the strings returned in dn and errmsg. One didn't, as the dn and errmsg were simply set to point to the strings held in the LDAP structure.
The expected problems introduced are differ depending on whether OpenLDAP ldap_get_lderrno(), it should be clear that problems are introduced. Simply put, despite the introduction of ldap_get_lderrno(), application developers need to be careful in porting to ensure that OpenLDAP offers compatible semantics to what they offer. And that likely requires reading, at a minimum, the documentation.
I rather developers simply port their code to use ldap_get_option(3) interface here, which because of inclusion in the LDAPEXT LDAP C API drafts (even through expired), are far more consistently implemented than routines such as ldap_get_lderrno().
I also oppose adding ldap_get_lderror().
Beyond this, I haven't reviewed the patch in detail. I think I'll leave that more current and more active in the development of OpenLDAP Software.
-- Kurt
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.
Hi. Your submission looks quite interesting. In fact, I've been trying to track down an issue for a while, which appeared to be a nasty mishandled concurrency issue of libldap within the proxy backends. Looking at your patch, we seem to have been touching similar portions of code. Our modifications are not final yet, as I'm waiting for the response of some apparently quite time consuming tests. In any case, we'll need to coordinate a bit when committing these changes. Just for the records, the whole thing started as ITS#6574. In any case, I'll start reviewing your patch soon.
Thanks, p.
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
On 08/20/10 05:01 PM, masarati@aero.polimi.it wrote:
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?
Yes, I believe that I have, but I agree that the new test060-mt-hot test case does not sufficiently test these mutexes. I will make some additional enhancements to the new test case to provide better code coverage results, and submit them as part of the next round of code review.
Doug.
-
Doug Leavitt -
Howard Chu -
Kurt Zeilenga -
masarati＠aero.polimi.it