Re: LDAP: bugfix and deprecated OpenLDAP API - Mailing list pgsql-hackers

From Magnus Hagander
Subject Re: LDAP: bugfix and deprecated OpenLDAP API
Date
Msg-id CABUevEwDQiAW3Tc+RTerGx46neAOrHhfsU5hQeWfHeXgTRLTkg@mail.gmail.com
Whole thread Raw
In response to Re: LDAP: bugfix and deprecated OpenLDAP API  (Albe Laurenz <laurenz.albe@wien.gv.at>)
List pgsql-hackers
On Mon, Oct 21, 2013 at 3:31 PM, Albe Laurenz <laurenz.albe@wien.gv.at> wrote:
Peter Eisentraut wrote:
>> --- 3511,3534 ----
>>      }
>>
>>      /*
>> !     * Perform an explicit anonymous bind.
>> !     * This is not necessary in principle, but we want to set a timeout
>> !     * of PGLDAP_TIMEOUT seconds and return 2 if the connection fails.
>> !     * Unfortunately there is no standard conforming way to do that.
>>       */
>
> This comment has become a bit confusing.  What exactly is nonstandard?
> Setting a timeout, or returning 2?  The code below actually returns 3.

I have improved the comment.

>> + #ifdef HAVE_LIBLDAP
>> +    /* in OpenLDAP, use the LDAP_OPT_NETWORK_TIMEOUT option */
>
> We don't use HAVE_LIBLDAP anywhere else to mean OpenLDAP.  Existing
> LDAP-related code uses #ifdef WIN32.

Changed.

> > + #else
>
> There should be a comment here indicating what this #else belongs to
> (#else /* HAVE_LIBLDAP */, or whatever we end up using).

Changed.

>> +    /* the nonstandard ldap_connect function performs an anonymous bind */
>> +    if (ldap_connect(ld, &time) != LDAP_SUCCESS)
>> +    {
>> +            /* error or timeout in ldap_connect */
>> +            free(url);
>> +            ldap_unbind(ld);
>> +            return 2;
>> +    }
>> + #endif
>
> here too

Changed.

> Bonus: Write a commit message for your patch.  (Consider using git
> format-patch.)

Suggested commit message is included in the patch.

Sorry about the huge delay in trying to get around to this one.

For the sake of the archives - I looked at the fact that the windows codepath always returns 2, whereas the unix codepath separates at least one case out as a return 3. I can't see a pattern in the windows return codes that would make it possible to do the same though, without explicitly listing different error codes mapping to each of them - so I don't think it's worth doing that.

Thus - applied, and backpatched all the way. Thanks!
 
(And I just realized I forgot to credit reviewers in the commit message. My apologies - I blame confusion after havnig to re-setup my windows build environments all over again..)

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

pgsql-hackers by date:

Previous
From: Magnus Hagander
Date:
Subject: Re: Tracking replication slot "blockings"
Next
From: Andres Freund
Date:
Subject: Re: Tracking replication slot "blockings"