You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Graham Leggett <mi...@sharp.fm> on 2001/07/31 00:23:00 UTC
[PATCH] LDAP support for apr-util
Hi all,
This is the initial landing of the LDAP support for apr-util.
It has the following purposes:
- Do the job of finding and linking to a suitable LDAP library
- Add an LDAP search and compare cache for extra performance
- Provide an LDAP connection cache so that LDAP connections can be
reused
- Other good things
The code is based on a port of an auth_ldap module by Dave Carrigan
released under the Apache licence.
The purpose of posting this code here is to get feedback from people on
what else needs to be done to this code before it could be accepted for
inclusion into APR-util.
Stuff that should work:
- If the --with-ldap flag is not specified on the ./configure line, the
LDAP components should not be built, and no errors should crop up. This
part of the library defaults to disabled.
- If the --with-ldap flag is specified, it should successfully find and
link to the OpenLDAP libraries (v1.2.x and v2.0.x), or the Netscape LDAP
libraries, or the new iPlanet LDAP libraries (untested).
If there is anything within the code that is either broken, or
implemented incorrectly, please let me know so this can be fixed.
The include file apr_ldap.h.in goes in the include/ directory. The rest
of the files go
in a new directory called ldap/
Regards,
Graham
--
-----------------------------------------
minfrin@sharp.fm "There's a moon
over Bourbon Street
tonight..."
Re: [PATCH] LDAP support for apr-util
Posted by Graham Leggett <mi...@sharp.fm>.
Justin Erenkrantz wrote:
> This line:
> LIBS="-l${ldaplib} ${extralib} $LIBS"
> would be replaced with:
> APR_ADDTO(LIBS, "-l${ldaplib} ${extralib}")
> m4 escaping rules may want it to be (I forget):
> APR_ADDTO(LIBS, [-l\${ldaplib} \${extralib}])
>
> AC_CHECK_LIB stays the same. This just prevents multiple instances of
> the same library from being inserted into the LIBS string (which is a
> real annoyance).
Ok, makes sense.
The trouble is that the APR_ADDTO macro is not avaiulable to APR-util.
Should it be? Should I just copy the macro out of APR and into APR-util?
> On Solaris, -R specifies "add this directory to the run-time linker
> search path." GNU ld has -rpath. This obviates the need for
> LD_LIBRARY_PATH and other hacks. Wherever you do -L, you should add -R
> (only if you are on Solaris). I'd also do -rpath on Linux, but that's
> me.
>
> APR_ADDTO(LDFLAGS, "-L/path/to/lib")
> if on Solaris, you also want to do: APR_ADDTO(LDFLAGS, "-R/path/to/lib")
> if on Linux, you also want to do: APR_ADDTO(LDFLAGS, "-Wl,-rpath /path/to/lib")
Would I specifically have to test for Solaris and then add the -R flag,
or can I get autoconf to do this for me?
> In my experience, LDAP code should be fairly fast. It really doesn't
> even depend on your LDAP server. I run OpenLDAP on a SparcStation 10 -
> that's not much fun (40MHz CPUs), but the lookups and queries are
> really quick. On current hardware, this is really a non-issue.
The code is there, and mature and stable - I'll try do a test both with
and without the cache code to see what a difference it makes.
Regards,
Graham
--
-----------------------------------------
minfrin@sharp.fm "There's a moon
over Bourbon Street
tonight..."
Re: [PATCH] LDAP support for apr-util
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Thu, Aug 02, 2001 at 02:22:53AM +0200, Graham Leggett wrote:
> Doing a grep for "-R" within APR finds this flag only in libtool - can
> libtool be used to make sure the libs are correctly specified?
I was wondering about that today as I was looking through libtool.
So, don't worry about it. If I want the LDAP libraries to link right
and I need the -R flag, I'll find some way of adding it. =)
-- justin
Re: [PATCH] LDAP support for apr-util
Posted by Graham Leggett <mi...@sharp.fm>.
Justin Erenkrantz wrote:
> On Solaris, -R specifies "add this directory to the run-time linker
> search path." GNU ld has -rpath. This obviates the need for
> LD_LIBRARY_PATH and other hacks. Wherever you do -L, you should add -R
> (only if you are on Solaris). I'd also do -rpath on Linux, but that's
> me.
>
> APR_ADDTO(LDFLAGS, "-L/path/to/lib")
> if on Solaris, you also want to do: APR_ADDTO(LDFLAGS, "-R/path/to/lib")
> if on Linux, you also want to do: APR_ADDTO(LDFLAGS, "-Wl,-rpath /path/to/lib")
Doing a grep for "-R" within APR finds this flag only in libtool - can
libtool be used to make sure the libs are correctly specified?
Regards,
Graham
--
-----------------------------------------
minfrin@sharp.fm "There's a moon
over Bourbon Street
tonight..."
Re: [PATCH] LDAP support for apr-util
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Tue, Jul 31, 2001 at 11:10:41AM +0200, Graham Leggett wrote:
> 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.
This line:
LIBS="-l${ldaplib} ${extralib} $LIBS"
would be replaced with:
APR_ADDTO(LIBS, "-l${ldaplib} ${extralib}")
m4 escaping rules may want it to be (I forget):
APR_ADDTO(LIBS, [-l\${ldaplib} \${extralib}])
AC_CHECK_LIB stays the same. This just prevents multiple instances of
the same library from being inserted into the LIBS string (which is a
real annoyance).
> > 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?
On Solaris, -R specifies "add this directory to the run-time linker
search path." GNU ld has -rpath. This obviates the need for
LD_LIBRARY_PATH and other hacks. Wherever you do -L, you should add -R
(only if you are on Solaris). I'd also do -rpath on Linux, but that's
me.
APR_ADDTO(LDFLAGS, "-L/path/to/lib")
if on Solaris, you also want to do: APR_ADDTO(LDFLAGS, "-R/path/to/lib")
if on Linux, you also want to do: APR_ADDTO(LDFLAGS, "-Wl,-rpath /path/to/lib")
> > > /* LDAP header files */
> > > #if APR_HAVE_LDAP_H
> > > #include <@w...@ldap.h>
> > > #endif
> > > #if APR_HAVE_LBER_H
> > > #include <@w...@lber.h>
> > > #endif
> > > #if APR_HAVE_LDAPSSL_H
> > > #include <@w...@ldap_ssl.h>
> > > #endif
> >
> > Couldn't we rely on setting CFLAGS/CPPFLAGS correctly instead?
>
> Do you have an example of how to do this?
If you set the CFLAGS and CPPFLAGS environment variables in your
autoconf script, those directories should be included in the Makefile
when you build. Therefore, the path in the #include directive is moot.
> 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.
In my experience, LDAP code should be fairly fast. It really doesn't
even depend on your LDAP server. I run OpenLDAP on a SparcStation 10 -
that's not much fun (40MHz CPUs), but the lookups and queries are
really quick. On current hardware, this is really a non-issue.
> > 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.
Yeah, get it in and then let's see if we can remove the need for
caching. My gut tells me that you won't really want it. I could be
wrong though.
> > 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?
Shouldn't.
I'd have the API in apr-util be essentially blocking, but the
underlying implementation may use asynchronous with timeouts. The
user of apr-util shouldn't have to deal with the asynchronous crap
(as far as he knows, it is synchronous). -- justin
Re: [PATCH] LDAP support for apr-util
Posted by Graham Leggett <mi...@sharp.fm>.
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 <@w...@ldap.h>
> > #endif
> > #if APR_HAVE_LBER_H
> > #include <@w...@lber.h>
> > #endif
> > #if APR_HAVE_LDAPSSL_H
> > #include <@w...@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..."
Re: [PATCH] LDAP support for apr-util
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Tue, Jul 31, 2001 at 10:04:24AM -0400, Bill Stoddard wrote:
> > 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.
> >
> >
>
> Isn't this only useful if your application is able to support event driven (or async) I/O?
> The connect to the LDAP server definitely needs to have a timeout, regardless.
I was thinking of something similar to the socket code. It uses the
asynchronous calls, but then waits for a timeout value for a response.
If it doesn't return in time, APR_TIMEUP is returned. I see no
fundamental reason why that model couldn't work with LDAP's async
calls.
(Truly async LDAP could be a nightmare to code though.) -- justin
Re: [PATCH] LDAP support for apr-util
Posted by Bill Stoddard <bi...@wstoddard.com>.
> 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.
>
>
Isn't this only useful if your application is able to support event driven (or async) I/O?
The connect to the LDAP server definitely needs to have a timeout, regardless.
Bill
Re: [PATCH] LDAP support for apr-util
Posted by Justin Erenkrantz <je...@ebuilt.com>.
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 <@w...@ldap.h>
> #endif
> #if APR_HAVE_LBER_H
> #include <@w...@lber.h>
> #endif
> #if APR_HAVE_LDAPSSL_H
> #include <@w...@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
Re: [PATCH] LDAP support for apr-util
Posted by Ryan Bloom <rb...@covalent.net>.
A couple of things.
1) This is FAR too large to review. It would be much easier if we started with something small,
and allowed the number of functions to grow as we need them.
2) There are C++ comments littered throughout the code. We can't use C++ comments, because
many compilers throw up on them.
3) The apr_ldap.h header file has a bunch of functions that aren't documented at all. No function
should ever be committed to APR or APR-util without docs.
4) The docs for the ldap structures won't generate doxygen docs. Again, no structure should ever
be committed that way.
That's it for now. This is not an exhaustive review, because the patch is too large to spend that
much time on.
Ryan
On Monday 30 July 2001 15:23, Graham Leggett wrote:
> Hi all,
>
> This is the initial landing of the LDAP support for apr-util.
>
> It has the following purposes:
>
> - Do the job of finding and linking to a suitable LDAP library
> - Add an LDAP search and compare cache for extra performance
> - Provide an LDAP connection cache so that LDAP connections can be
> reused
> - Other good things
>
> The code is based on a port of an auth_ldap module by Dave Carrigan
> released under the Apache licence.
>
> The purpose of posting this code here is to get feedback from people on
> what else needs to be done to this code before it could be accepted for
> inclusion into APR-util.
>
> Stuff that should work:
>
> - If the --with-ldap flag is not specified on the ./configure line, the
> LDAP components should not be built, and no errors should crop up. This
> part of the library defaults to disabled.
>
> - If the --with-ldap flag is specified, it should successfully find and
> link to the OpenLDAP libraries (v1.2.x and v2.0.x), or the Netscape LDAP
> libraries, or the new iPlanet LDAP libraries (untested).
>
> If there is anything within the code that is either broken, or
> implemented incorrectly, please let me know so this can be fixed.
>
> The include file apr_ldap.h.in goes in the include/ directory. The rest
> of the files go
> in a new directory called ldap/
>
> Regards,
> Graham
Ryan
_____________________________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
-----------------------------------------------------------------------------