> On 25 Nov 2025, at 00:28, Chao Li <[email protected]> wrote: > > Hi Daniel, > > None of my comment on v9 are addressed in v10:
I do apologise, I was so focused on fixing Jacob's tests that I forgot about
addressing these. Please find the attached v11 with your comments addressed.
Thank you for all your review, much appreciated!
>> 1 - commit message
>> ```
>> Experimental support for serverside SNI support in libpq, a new config
>> file $datadir/pg_hosts.conf is used for configuring which certicate and
>> ```
>>
>> Typo: certicate -> certificate
Fixed. I also reworded the commit message from saying experimental since we
don't have a concept of experimental features really.
>> 2 - be-secure-common.c
>> ```
>> +run_ssl_passphrase_command(const char *prompt, bool is_server_start, char
>> *buf, int size, void *userdata)
>> {
>> int loglevel = is_server_start ? ERROR : LOG;
>> char *command;
>> FILE *fh;
>> int pclose_rc;
>> size_t len = 0;
>> + char *cmd = (char *) userdata;
>> ```
>>
>> Cmd is only passed to replace_percent_placeholders(), and the function take
>> a "const char *” argument, so we can define cmd as “const char *”.
Fixed.
>> 2 - be-secure-common.c
>> ```
>> + tokenize_auth_file(HostsFileName, file, &hosts_lines, LOG, 0);
>> +
>> + foreach(line, hosts_lines)
>> + {
>> + TokenizedAuthLine *tok_line = (TokenizedAuthLine *) lfirst(line);
>> +
>> + if (tok_line->err_msg != NULL)
>> + {
>> + ok = false;
>> + continue;
>> + }
>> +
>> + if ((newline = parse_hosts_line(tok_line, LOG)) == NULL)
>> + {
>> + ok = false;
>> + continue;
>> + }
>> +
>> + parsed_lines = lappend(parsed_lines, newline);
>> + }
>> +
>> + free_auth_file(file, 0);
>> ```
>>
>> When I read this function, I thought to raise a comment for freeing
>> hosts_lines. However, then I read be-secure-openssl.c, I saw the
>> load_hosts() is called within a transient hostctx, so it doesn’t have to
>> free memory pieces. Can we explain that in the function comment? Otherwise
>> other reviewers and future code readers may have the same confusion.
I expanded the comment, and while there also improved the error reporting from
the function by returning a bool indicating status as well as the list (since
NIL was both empty-file and error).
>> 3 - be-secure-openssl.c
>> ```
>> int
>> @@ -759,6 +933,9 @@ be_tls_close(Port *port)
>> pfree(port->peer_dn);
>> port->peer_dn = NULL;
>> }
>> +
>> + Host_context = NULL;
>> + SSL_context = NULL;
>> }
>> ```
>>
>> Do we need to free_contexts() here? When be_tls_init() is called again,
>> contexts will anyway be freed, so I guess earlier free might be better?
I don't think so, be_tls_close is only for closing the session.
> I reviewed v10 again, and got some a few more comments:
>
> 5 - runtime.sgml
> ```
> + in <filename>pg_hba.conf</filename>. <replaceable>hostname</replaceable>
> + is matched against the hostname TLS extension in the SSL handshake.
> ```
>
> In the patch code, default context uses hostname “*”, should we explain “*”
> here in the doc?
I don't think we should since we don't want anyone to configure a host with
'*'. That does bring up a good point though, and I added a check in the
parsing to ensure that such wildcard hostnames cause failures in parsing if
found in pg_hosts.
> 6 - runtime.sgml
> ```
> + <filename>pg_hosts.conf</filename>, which is stored in the clusters
> + data directory. The hosts configuration file contains lines of the
> general
> ```
>
> Typo: clusters => cluster’s
Fixed.
> 7 - runtime.sgml
> ```
> + will only be used to for the handshake until the hostname is inspected,
> it
> ```
>
> “Used to for” => “used for"
Fixed.
> 8 - Cluster.pm
> ```
> +matching the specified pattern. If the pattern matches agsinst the logfile a
> ```
>
> Typo: agsinst => against
Fixed.
--
Daniel Gustafsson
v11-0001-Serverside-SNI-support-for-libpq.patch
Description: Binary data
