apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Graham Leggett <minf...@sharp.fm>
Subject Re: [PATCH] LDAP support for apr-util
Date Tue, 31 Jul 2001 09:10:41 GMT
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 <@with_ldap_include@ldap.h>
> > #endif
> > #if APR_HAVE_LBER_H
> > #include <@with_ldap_include@lber.h>
> > #endif
> > #if APR_HAVE_LDAPSSL_H
> > #include <@with_ldap_include@ldap_ssl.h>
> > #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
-- 
-----------------------------------------
minfrin@sharp.fm		"There's a moon
					over Bourbon Street
						tonight..."
Mime
View raw message