Re: LDAP: bugfix and deprecated OpenLDAP API

On Mon, Oct 21, 2013 at 3:31 PM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)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/