On 11/28/19 10:25 PM, Greg Nancarrow wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: tested, passed
> Spec compliant: tested, passed
> Documentation: tested, failed
>
> Hi Andrew,
>
> I've reviewed your "libpq sslpassword parameter and callback function" patch
(0001-libpq-sslpassword-der-support.patch),and only found a few minor things (otherwise it looked good to me):
>
> 1) There's a few trailing white-space warnings on patch application (from where you modified to skip 2 of the
tests):
> git apply 0001-libpq-sslpassword-der-support.patch
> 0001-libpq-sslpassword-der-support.patch:649: trailing whitespace.
> # so they don't hang. For now they are not performed.
> 0001-libpq-sslpassword-der-support.patch:659: trailing whitespace.
>
> warning: 2 lines add whitespace errors.
>
>
> 2) src/interfaces/libpq/libpq-fe.h
> The following portion of the comment should be removed.
>
> + * 2ndQPostgres extension. If you need to be compatible with unpatched libpq
> + * you must dlsym() these.
>
> 3) Documentation for the "PQsslpassword" function should be added to the libpq "33.2 Connection Status Functions"
section.
>
>
> I made the following notes about how/what I reviewed and tested:
>
> - Applied patch and built Postgres (--with-openssl --enable-tap-tests), checked build output
> - Checked patch code modifications (format, logic, memory usage, efficiency, corner cases etc.)
> - Built documentation and checked updated portions (format, grammar, details, completeness etc.)
> - Checked test updates
> - Ran updated contrib/dblink tests - confirmed all passed
> - Ran updated SSL (TAP) tests - confirmed all passed
> - Ran "make installcheck-world", as per review requirements
> - Wrote small libpq-based app to test:
> - new APIs (PQsslpassword, PQsetSSLKeyPassHook, PQgetSSLKeyPassHook, PQdefaultSSLKeyPassHook)
> - passphrase-protected key with/without patch
> - patch with/without new key password callack
> - patch and certificate with/without pass phrase protection on key
> - default callback, callback delegation
> - PEM/DER keys
>
>
Thanks, nice thorough review.
Here's an updated patch that I think fixes all the things you mentioned.
I plan to commit this tomorrow.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
