Thanks for the review! I have fixed all your feedback in the attached patch.
On 11/03/2016 04:24 PM, Michael Banck wrote:
> It does not update the documentation, I think at least section 18.9
> "Secure TCP/IP Connections with SSL" needs updating as it says: "The
> files server.key, server.crt, root.crt, and root.crl (or their
> configured alternative names) are only examined during server start; so
> you must restart the server for changes in them to take effect".
Changed this.
> However see below for segfaults during testing.
Fixed, this was due to me not setting SSL_Context to NULL after freeing it.
>> [...]
>
> I am not sure this '!!' operator is according to project policy, it
> seems to be not used elsewhere in the codebase except for imported or
> auto-generated code. At least there should be a comment here, methinks.
I changed the code to compare with '\0' instead.
> [...]
> Missing semicolon at the end of the line.
> [...]
> Opening braces should be on the next line.
> [...]
> Same.
Fixed.
>> [...]
>
> All the delarations above this one are global variables for GUCs (see
> postmaster.h, lines 16-31). So ISTM this static variable declaration
> should be introduced by a comment in order to logically seperate it from
> the previous ones, like the following static variables are:
Fixed.
>> [...]
>
> This hunk baffled me at first because two lines below your added
> secure_destroy() declaration, the same line is already present. I did
> some digging and it turns out we had a secure_destroy() in the ancient
> past, but its implementation got removed in 2008 in 4e8162865 as there
> were no (more) users of it, however, the declaration was kept on until
> now.
>
> So this hunk should be removed I guess.
Removed.
Andreas