I'm going to take a stab on reviewing this (inline). I've written enough LDAP stuff in my life to know the code by heart. (I've written both C and Java bindings - yummy...)
So, definitely +1 in concept on the whole LDAP in apr-util thing. If it is of any help, you may find what I've done before at: http://www.ucf.ics.uci.edu/~jerenk/ldapprogs.tar.gz The key files are LDAPUtil.cc and LDAPUtil.h - the rest of the files use these two to contact LDAP servers. It says it is C++, but it is darn close to C - I had a few constructs I never bothered to make C. I haven't touched it in a long time - I'm probably a much better C programmer now. (Don't worry about the license - I'd license it under BSD if it helps out...) > +dnl > +dnl Find a particular LDAP library > +dnl > +AC_DEFUN(APU_FIND_LDAPLIB,[ > + if test ${apr_have_ldap} != "1"; then > + ldaplib=$1 > + extralib=$2 > + unset ac_cv_lib_${ldaplib}_ldap_init > + apr_have_ldap="1" > + AC_CHECK_LIB(${ldaplib}, ldap_init, > + [ > +# 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. > + > + > +dnl > +dnl APU_FIND_LDAP: figure out where LDAP is located > +dnl > +AC_DEFUN(APU_FIND_LDAP,[ > + > +echo $ac_n "${nl}checking for ldap support...${nl}" > + > +apr_have_ldap="0" > +apr_have_ldap_h="0" > +apr_have_lber_h="0" > +apr_have_ldapssl_h="0" > +apr_have_ldapssl_install_routines="0" > +apr_have_ldap_start_tls_s="0" > + > +AC_ARG_WITH(ldap-include, --with-ldap-include=path path to ldap include > files with trailing slash) > +AC_ARG_WITH(ldap-lib, --with-ldap-lib=path path to ldap lib file) > +AC_ARG_WITH(ldap, --with-ldap=library ldap library to use, > + [ > + if test -n "$with_ldap_include"; then > + CPPFLAGS="-I$with_ldap_include $CPPFLAGS" > + fi > + if test -n "$with_ldap_lib"; then > + LDFLAGS="-L$with_ldap_lib $LDFLAGS" > + fi > + > + LIBLDAP="$withval" > + if test "$LIBLDAP" = "yes"; then > +dnl The iPlanet C SDK 5.0 is as yet untested... > + APU_FIND_LDAPLIB("ldap50", "-lnspr4 -lplc4 -lplds4 -liutil50 -llber50 > -lldif50 -lnss3 -lprldap50 -lssl3 -lssldap50") # Generic > + APU_FIND_LDAPLIB("ldapssl41", "-lnspr3 -lplc3 -lplds3") # Generic > + APU_FIND_LDAPLIB("ldapssl41", " -lnspr3 -lplc3 -lplds3 -lsocket -lnsl > -lmt") # Solaris > + APU_FIND_LDAPLIB("ldapssl41", " -lnspr3 -lplc3 -lplds3 -lpthread") # > Linux > + APU_FIND_LDAPLIB("ldapssl40") > + APU_FIND_LDAPLIB("ldapssl40", "-lpthread") > + APU_FIND_LDAPLIB("ldapssl30") > + APU_FIND_LDAPLIB("ldapssl30", "-lpthread") > + APU_FIND_LDAPLIB("ldapssl20") > + APU_FIND_LDAPLIB("ldapssl20", "-lpthread") > + APU_FIND_LDAPLIB("ldap", "-llber") > + else > + AC_CHECK_LIB($LIBLDAP,main,apr_have_ldap="1",apr_have_ldap="0",-llber) > + fi > + > + test $apr_have_ldap != "1" && AC_MSG_ERROR(could not find an LDAP > library) > + AC_CHECK_LIB(lber, ber_init, , AC_CHECK_LIB(lber, ber_init), -lnsl) > + > + AC_CHECK_HEADERS(ldap.h, apr_have_ldap_h="1", apr_have_ldap_h="0") > + AC_CHECK_HEADERS(lber.h, apr_have_lber_h="1", apr_have_lber_h="0") > + AC_CHECK_HEADERS(ldap_ssl.h, apr_have_ldapssl_h="1", > apr_have_ldapssl_h="0") > + > + AC_SUBST(apr_have_ldap) > + AC_SUBST(apr_have_ldap_h) > + AC_SUBST(apr_have_lber_h) > + AC_SUBST(apr_have_ldapssl_h) > + AC_SUBST(with_ldap_include) > + AC_SUBST(apr_have_ldapssl_install_routines) > + AC_SUBST(apr_have_ldap_start_tls_s) > + ]) > + > +]) 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. 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). Also because -lnsl should already be present, I think you can make the following > + AC_CHECK_LIB(lber, ber_init, , AC_CHECK_LIB(lber, ber_init), -lnsl) be: > + AC_CHECK_LIB(lber, ber_init) (You may not have synced up since rbb nuked apr-util's build system - apr-util now relies completely on apr - as it should.) Continuing on....(the rest of the configure.in file has some formatting changes that aren't a part of the patch...minor quibble...) > #define APR_HAVE_LDAP_H @apr_have_ldap_h@ > #define APR_HAVE_LBER_H @apr_have_lber_h@ > #define APR_HAVE_LDAPSSL_H @apr_have_ldapssl_h@ > #define APR_HAVE_NETSCAPE_SSL @apr_have_ldapssl_install_routines@ > #if !APR_HAVE_NETSCAPE_SSL > #undef APR_HAVE_NETSCAPE_SSL > #endif > #define APR_HAVE_START_TLS @apr_have_ldap_start_tls_s@ > #if !APR_HAVE_START_TLS > #undef APR_HAVE_START_TLS > #endif > #define APR_HAVE_LDAP @apr_have_ldap@ > #if !APR_HAVE_LDAP > #undef APR_HAVE_LDAP > #endif 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. > /* this whole thing disappears if LDAP is not enabled */ > #ifdef APR_HAVE_LDAP This should be: #if APU_HAS_LDAP (in my world...) > /* 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? > /* XXXXXXXXXXXXXX */ > #include <stdarg.h> > #include <sys/types.h> > #include <time.h> There are some APR_HAVE_SYS_TYPES_H macros you can use here. > /* apr_ldap_cache_mgr.c */ > > void apr_ald_free(void *ptr); > void *apr_ald_alloc(int size); > char *apr_ald_strdup(char *s); > unsigned long apr_ald_hash_string(int nstr, ...); > void apr_ald_cache_purge(apr_ald_cache_t *cache); > apr_url_node_t *apr_ald_create_caches(apr_ldap_state_t *s, char *url); > apr_ald_cache_t *apr_ald_create_cache(unsigned long maxentries, > unsigned long (*hashfunc)(void *), > int (*comparefunc)(void *, void *), > void * (*copyfunc)(void *), > void (*freefunc)(void *)); > void apr_ald_destroy_cache(apr_ald_cache_t *cache); > void *apr_ald_cache_fetch(apr_ald_cache_t *cache, void *payload); > void apr_ald_cache_insert(apr_ald_cache_t *cache, void *payload); > void apr_ald_cache_remove(apr_ald_cache_t *cache, void *payload); > void apr_ald_cache_display_stats(apr_pool_t *p, apr_ald_cache_t *cache, > char *name, char **stats); > > > #endif /* APR_HAVE_LDAP */ > #endif /* APR_LDAP_H */ 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. =-) 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. > if ((result = ldap_simple_bind_s(ldc->ldap, ldc->binddn, ldc->bindpw)) > == LDAP_SERVER_DOWN) { > /* couldn't connect - try again */ > apr_ldap_close_connection(ldc); > goto start_over; > } > > if (result != LDAP_SUCCESS) { > /* LDAP fatal error occured */ > apr_ldap_close_connection(ldc); > return result; > } > > /* note how we are bound */ > ldc->boundas = bind_system; > > return LDAP_SUCCESS; > } 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). 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
