Justin Erenkrantz wrote:
> > +# LIBS="-I${ldaplib} ${extralib} $LIBS"
> > + LIBS="-l${ldaplib} ${extralib} $LIBS"
> > + APRUTIL_EXPORT_LIBS="$APRUTIL_EXPORT_LIBS -l${ldaplib} ${extralib}"
> > + AC_CHECK_LIB(${ldaplib}, ldapssl_install_routines,
> > apr_have_ldapssl_install_routines="1", , ${extralib})
> > + AC_CHECK_LIB(${ldaplib}, ldap_start_tls_s,
> > apr_have_ldap_start_tls_s="1", , ${extralib})
> > + ],
> > + apr_have_ldap="0", ${extralib})
> > + fi
> > +])
>
> You probably want APR_ADDTO here. This eliminates duplicate values
> getting set into LIBS. You have a test below that sets -lnsl -lsocket
> which should already be present in LIBS on Solaris. More on that in a
> sec.
Would APR_ADDTO replace APR_CHECK_LIB? I just used macros that are
available as part of Autoconf - I am not familiar with any of the APR
macros.
> > + APU_FIND_LDAPLIB("ldapssl41", " -lnspr3 -lplc3 -lplds3 -lpthread") #
> > Linux
> If APR is doing its job right, you shouldn't need the OS specific
> libraries here. If the pthread, nsl, socket libraries are needed, they
> should already be available by the time apr-util's configure is executed.
Cool - I'll take the extra tests out.
> You will also need to add the -R flags for Solaris (not sure the best
> way to do this - be nice if APR had a macro like APR_ADDLIB which also
> added the -R flag when needed).
What does the -R flag do, and how should it be added?
> I'm not clear on the naming. APR_HAVE_LDAP should probably be
> APR_HAS_LDAP. rbb probably knows the naming system better than
> anyone else. It confuses the hell out of me right now. And,
> the prefix should be APU not APR - this stuff is in apr-util.
Will fix - the naming is a bit of a mess at the moment, I just took the
stuff in the existing code and added an APR_ in fron of it. Will go
through these and fix them up properly.
> > /* LDAP header files */
> > #if APR_HAVE_LDAP_H
> > #include <@[EMAIL PROTECTED]>
> > #endif
> > #if APR_HAVE_LBER_H
> > #include <@[EMAIL PROTECTED]>
> > #endif
> > #if APR_HAVE_LDAPSSL_H
> > #include <@[EMAIL PROTECTED]>
> > #endif
>
> Couldn't we rely on setting CFLAGS/CPPFLAGS correctly instead?
Do you have an example of how to do this?
> I'm not really clear what this apr_ald_cache_foo magic is for. LDAP
> should be optimized for lookups, so I'm not sure I understand the
> need for the client-side caching. Optimize the indicies in LDAP
> instead. =-)
The client side caching was originally part of the auth_ldap module
where this code originates.
The idea behind it is that various lookup and compare operations are
cached, rather than the results of actual LDAP queries. It has a large
performance advantage, as often repeated queries (is this
username/password correct, is this dn a member of this group) are not
repeated on every hit.
> If you really want to cache it locally, I'd use anonymous DBMs or
> something more abstract (i.e. not at this level, but at the level
> where this code would be called). I'm not sold on needing to have
> this cache in the general case.
I'd like to review this cache stuff down the line - as it currently uses
malloc() and free(). Either a DBM based or SMS based cache might do the
trick.
In the meantime, I am busy trying to hide the cache entirely from the
caller so we can fiddle on the backend without breaking things.
> For reliability reasons, you probably want to use asynchronous bindings.
>
> Do:
> ldap_init
> ldap_simple_bind
> ldap_result
>
> If the LDAP server is unreachable, you'll block waiting for the server
> to respond. It's really preferable to use asynchronous bindings (I've
> used 1 second timeouts in the past).
Hmmm - this is better, yes. Will change this.
How would this impact locking?
> What I've typically done (see above URL) is to do the connection to the
> server asynchronously and then everything else synchronously. If the
> server dies in the middle, you are screwed. But, to be done "right,"
> you probably want to use asynchronous everywhere. Something like the
> timeout values in APR for sockets. I dunno - it's a thought.
>
> Okay. Very cool. Keep me posted. =-) -- justin
The chopping and hacking continues :)
Regards,
Graham
--
-----------------------------------------
[EMAIL PROTECTED] "There's a moon
over Bourbon Street
tonight..."
smime.p7s
Description: S/MIME Cryptographic Signature
