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
Regards,
Greg
