On 03.04.25 17:51, Daniel Gustafsson wrote:
>> On 1 Apr 2025, at 22:22, Daniel Gustafsson <daniel@yesql.se> wrote:
>>
>> I took another pass at this one and did a few small tweaks, so I would to take
>> it for another spin across CI before looking at committing it. There are no
>> functionality changes, only polish.
>
> Committed, after another round of testing and looking.
A few more observations on this code:
What is the meaning of this:
+ old_umask = umask(077);
+ fd = open(conn->sslkeylogfile, O_WRONLY | O_APPEND | O_CREAT, 0600);
+ umask(old_umask);
If open() already specifies the file mode, do we still need umask()?
Maybe I'm missing something.
Also, we usually use symbols for the modes.
+ libpq_append_conn_error(conn, "could not open ssl keylog file
%s: %s",
+ conn->sslkeylogfile, pg_strerror(errno));
pg_strerror() is not thread-safe, so it shouldn't be used here.
Actually, pg_strerror() is not meant for direct use at all. Probably
easiest to just use %m.
Also, I can't actually trigger these errors. This call just sticks the
errors into the connection structure, but if the connection is otherwise
successful, nothing will print the error. Maybe the best we can do is
print the error to stderr, like similarly in initialize_SSL().
