Re: svn commit: r1917094 [2/3] - in /apr/apr/trunk: ./ build/ docs/ include/ include/private/ ldap/ misc/unix/ test/

On 8/20/24 10:27 AM, Ruediger Pluem wrote:
> 
> 
> On 4/18/24 9:32 AM, [email protected] wrote:
>> Added: apr/apr/trunk/ldap/apr_ldap.c
>> URL: http://svn.apache.org/viewvc/apr/apr/trunk/ldap/apr_ldap.c?rev=1917094&view=auto
>> ==============================================================================
>> --- apr/apr/trunk/ldap/apr_ldap.c (added)
>> +++ apr/apr/trunk/ldap/apr_ldap.c Thu Apr 18 07:32:13 2024
> 
>> +
>> +
>> +
>> +APU_DECLARE_LDAP(apr_status_t) apr_ldap_connect(apr_pool_t *pool,
>> +                                                apr_ldap_t *ldap,
>> +                                                apr_interval_time_t timeout,
>> +                                                apu_err_t *err)
>> +{
>> +    LDAP *ld = ldap->ld;
>> +
>> +#if APR_HAS_MICROSOFT_LDAPSDK
>> +    struct timeval tv, *tvptr;
>> +
>> +    if (timeout < 0) {
>> +        tvptr = NULL;
>> +    }
>> +    else {
>> +        tv.tv_sec = (long) apr_time_sec(timeout);
>> +        tv.tv_usec = (long) apr_time_usec(timeout);
>> +        tvptr = &tv;
>> +    }
>> +
>> +    err->rc = ldap_connect(ld, tvptr);
>> +
>> +#else
>> +
>> +#ifdef LDAP_OPT_NETWORK_TIMEOUT
>> +    {
>> +        struct timeval tv, *tvptr;
>> +
>> +        if (timeout < 0) {
>> +            tvptr = NULL;
>> +        }
>> +        else {
>> +            tv.tv_sec = (long) apr_time_sec(timeout);
>> +            tv.tv_usec = (long) apr_time_usec(timeout);
>> +            tvptr = &tv;
>> +        }
>> +
>> +        err->rc = ldap_set_option(ldap->ld, LDAP_OPT_NETWORK_TIMEOUT, tvptr);
>> +        if (err->rc != LDAP_SUCCESS) {
>> +            err->msg = ldap_err2string(err->rc);
>> +            err->reason = "LDAP: Could not set network timeout";
>> +            return APR_EINVAL;
>> +        }
>> +    }
>> +#endif
>> +
>> +    err->rc = ldap_connect(ld);
> 
> This code is compiled with openldap and ldap_connect does not seem to be present with openldap at least not with 2.4.46 which is
> delivered with RedHat 8. I can see it though in openldap 2.6.6 which is delivered with RedHat 9
> 
> ldap/apr_ldap.c: In function ‘apr__ldap_connect’:
> ldap/apr_ldap.c:1091:15: warning: implicit declaration of function ‘ldap_connect’; did you mean ‘ldap_cancel’?
> [-Wimplicit-function-declaration]
>      err->rc = ldap_connect(ld);
>                ^~~~~~~~~~~~
>                ldap_cancel

Thanks to r1924295 this compiles now, but it creates just a stub for apr_ldap_connect that always returns APR_ENOTIMPL.
As apr_ldap_connect is a central function of the new API I think it would be better to fail during configure time
if ldap_connect is not found instead of creating a binary with an unusable API. Maybe a check for an appropriate version
of openldap (>= 2.5 looks like to be fine) in case openldap is the library being used would be also helpful to give the
user compiling the code a better hint what is required to make the API work.

Regards

Rüdiger