apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jerenkra...@ebuilt.com>
Subject Re: [PATCH] LDAP support for apr-util
Date Tue, 31 Jul 2001 07:43:44 GMT
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 <@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?

> /* 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


Mime
View raw message