You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Paul Querna <ch...@force-elite.com> on 2005/04/26 18:11:34 UTC

Re: svn commit: r160707 - in httpd/httpd/branches/2.0.x: CHANGES STATUS docs/manual/mod/mod_ldap.xml include/util_ldap.h modules/experimental/util_ldap.c

bnicholes@apache.org wrote:
> Author: bnicholes
> Date: Sat Apr  9 12:00:18 2005
> New Revision: 160707
> 
> URL: http://svn.apache.org/viewcvs?view=rev&rev=160707
> Log:
> Added a new LDAPConnectionTimeout directive to util_ldap so that the socket connection timeout value is configurable.
> 
> Reviewed by: bnicholes, trawick, jim
....
> @@ -1379,6 +1404,7 @@
>  
>      void *data;
>      const char *userdata_key = "util_ldap_init";
> +    struct timeval timeOut = {10,0};    /* 10 second connection timeout */
>  
>      /* util_ldap_post_config() will be called twice. Don't bother
>       * going through all of the initialization on the first call
> @@ -1603,6 +1629,20 @@
>                           "LDAP: SSL support unavailable" );
>      }
>      
> +#ifdef LDAP_OPT_NETWORK_TIMEOUT
> +    if (st->connectionTimeout > 0) {
> +        timeOut.tv_sec = st->connectionTimeout;
> +    }
> +
> +    if (st->connectionTimeout >= 0) {
> +        rc = ldap_set_option(NULL, LDAP_OPT_NETWORK_TIMEOUT, (void *)&timeOut);
> +        if (APR_SUCCESS != rc) {
> +            ap_log_error(APLOG_MARK, APLOG_ERR, 0, s,
> +                             "LDAP: Could not set the connection timeout" );
> +        }
> +    }
> +#endif
> +
>      return(OK);
>  }
>  
....

It looks like this change is causing crashes with PHP:
http://issues.apache.org/bugzilla/show_bug.cgi?id=34618
http://issues.apache.org/bugzilla/show_bug.cgi?id=34620

I think the call to ldap_set_option is the cause.

Quoting from http://docs.sun.com/source/816-5616-10/function.htm#24534

"""If NULL, you are setting the default options that will apply to any
new LDAP connection handles that are subsequently created.  """

Doesn't it seem bad to set this globally?  We are trampling on other
citizens inside the process.

Why isn't this call to ldap_set_option done per-LDAP context?

-Paul

Re: svn commit: r160707 - in httpd/httpd/branches/2.0.x: CHANGES STATUS docs/manual/mod/mod_ldap.xml include/util_ldap.h modules/experimental/util_ldap.c

Posted by Graham Leggett <mi...@sharp.fm>.
Paul Querna wrote:

> It looks like this change is causing crashes with PHP:
> http://issues.apache.org/bugzilla/show_bug.cgi?id=34618
> http://issues.apache.org/bugzilla/show_bug.cgi?id=34620
> 
> I think the call to ldap_set_option is the cause.
> 
> Quoting from http://docs.sun.com/source/816-5616-10/function.htm#24534
> 
> """If NULL, you are setting the default options that will apply to any
> new LDAP connection handles that are subsequently created.  """
> 
> Doesn't it seem bad to set this globally?  We are trampling on other
> citizens inside the process.
> 
> Why isn't this call to ldap_set_option done per-LDAP context?

I think it probably should be done per context when each connection is 
created, rather than set globally.

+1.

Regards,
Graham
--

Re: svn commit: r160707 - in httpd/httpd/branches/2.0.x: CHANGES STATUS docs/manual/mod/mod_ldap.xml include/util_ldap.h modules/experimental/util_ldap.c

Posted by Graham Leggett <mi...@sharp.fm>.
Paul Querna wrote:

> It looks like this change is causing crashes with PHP:
> http://issues.apache.org/bugzilla/show_bug.cgi?id=34618
> http://issues.apache.org/bugzilla/show_bug.cgi?id=34620
> 
> I think the call to ldap_set_option is the cause.
> 
> Quoting from http://docs.sun.com/source/816-5616-10/function.htm#24534
> 
> """If NULL, you are setting the default options that will apply to any
> new LDAP connection handles that are subsequently created.  """
> 
> Doesn't it seem bad to set this globally?  We are trampling on other
> citizens inside the process.
> 
> Why isn't this call to ldap_set_option done per-LDAP context?

I think it probably should be done per context when each connection is 
created, rather than set globally.

+1.

Regards,
Graham
--