On Wed, Sep 20, 2017 at 7:57 AM, Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote: > In the 0001 patch, I would move the ldap_unbind() calls after the > ereport(LOG) calls. We do all the other resource cleanup (pfree() etc.) > after the ereport() calls, so it would be weird to do this one > differently. Also, in the second patch you move one of the > ldap_unbind() calls down anyway.
Fair point. In that case there are a few others we should consider
moving down too for consistency, like in the attached.
> In the 0002 patch, I think this is a bit repetitive and could be
> refactored even more. The end result could look like
>
> ereport(LOG,
> (errmsg("blah"),
> errdetail_for_ldap(ldap)));
> ldap_unbind(ldap);
Thanks, that is much tidier. Done that way in the attached.
Here also is a small addition to your TAP test which exercises the
non-NULL code path because slapd rejects TLS by default with a
diagnostic message. I'm not sure if this is worth adding, since it
doesn't actually verify that the code path is reached (though you can
see that it is from the logs).
--
Thomas Munro
http://www.enterprisedb.com
0001-Improve-LDAP-cleanup-code-in-error-paths.patch
Description: Binary data
0002-Log-diagnostic-messages-if-errors-occur-during-LDAP-.patch
Description: Binary data
0003-Add-a-regression-test-to-log-an-LDAP-diagnostic-mess.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
