On Sun, 26 Jun 2011, Graham Leggett wrote:

On 25 Jun 2011, at 11:24 PM, Stefan Fritsch wrote:

This is not so, to fix this, you would need to wrap every single
LDAP API function call[1] in an optional function, and if you did
that, you would solve the problem that caused you to want to
remove apr_ldap from APR in the first place, making the whole
exercise pointless - you may as well just have fixed apr-ldap in
place.

No, that is not correct. You only need to wrap all ap_ldap_* calls and
make the depending modules (mod_authnz_ldap, mod_vhost_ldap, ...) link
to the ldap libraries themselves.

This is one of the key reasons that got apr_ldap evicted from APR in the first place.

Nobody said that this would be magically fixed by moving the stuff to HTTPD. But it means that the apr libraries are no longer involved in the mess, which is already very helpful for distributions like Debian. With the apr 1.x situation, an ABI break (soname change) in openldap means we have to jump through hoops to prevent crashes when httpd uses one version of openldap and apr uses a different version.

If the API is not good enough for APR, then the API is not good enough for httpd. We must fix this problem, not shift it around.

Moving the ldap_init code from mod_ldap into apr with httpd 2.2.x
broke support for Solaris LDAP (PR 42682), which is a regression from
2.0.x. Fixing that would require changes both in mod_ldap and in apr-
ldap. It says a lot that mod_ldap in 2.2.x does not use
apr_ldap_init() in the way it is recommended in the documentation
(setting secure if ldaps is wanted without client certificates).
Because if it did, it would not work, because the apr_ldap code is
buggy.

Looking at this, the fix involves teaching mod_ldap to pass the correct parameter to apr_ldap_ssl_init(), it doesn't require a change to the API that I can see.

Changing mod_ldap in this simple way breaks with OpenLDAP. So it's not that simple, unfortunately. If we make any changes to apr_ldap that require a different calling order of the functions or change the semantics in every other way, that's a change in the API. Even if all function prototypes stay the same.

The long term fix is to hide the LDAP structure within something like apr_ldap_t, and then lazy init when necessary. This is why apr_ldap_ssl_init() is marked deprecated in v1.x.

To address the comment in the PR about only supporting openssl, we support the following distinct LDAP toolkits across a myriad of platforms:

I think you misread that comment. The patch attached to the PR makes it work with Solaris LDAP but breaks with OpenLDAP, therefore it is not acceptable.

- OpenLDAP
- Solaris
- Novell
- Microsoft
- Netscape
- Mozilla
- Tivoli
- z/OS

Do we still work after the API was moved? For MacOSX at least, no, for the others, particularly many of the smaller platforms, probably not.

But if we have ap_ldap_init in HTTPD, we can work around changes in behaviour by modifying how mod_ldap calls the API. This is not possible with apr_ldap_init because it must continue to work with the unchanged, released versions of 2.2.x's mod_ldap. Therefore moving the ldap code into HTTPD gives more flexibility.

If MacOSX is broken, MacOSX users/devs should complain and provide more info. I couldn't find a mail about this.

But this highlights another problem with how we deal ldap: There is no way a developer can test changes on all or even on a significant subset of the supported toolkits. This makes it very hard to do any changes at all. Ideally we would only support toolkits/plattforms that are available at the ASF for testing.

Doing these changes in APR 1.x in a way that does not break an
unmodified 2.2.x mod_ldap with some LDAP library is next to
impossible. So moving the apr_ldap stuff as ap_ldap into httpd and
therefore making it possible to change the API is an advantage.

That's incorrect, it's not possible to change the API - when we release httpd v2.4.0, that's the API baked, and you don't get a chance fix this.

But 2.4.0 is not yet released. Until then we can change things. And if something breaks, we can fix it in 2.4.1. This is much less severe than if we break APR and affect all HTTPD 2.2.x user, too.

It is not fair on end users who have waited ages for httpd v2.4.0 to suddenly force them to wait even longer while we fix the APIs in APR. It is even worse to expect end users to deal with problems caused by APIs dumped in at the last minute without a proper stabilisation process before we make a major release.

Reply via email to