> On 26 Jun 2025, at 22:49, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> 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:
Thanks for review!
> 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.
No, I think that's a leftover from a previous version which I missed. I can't
see that being required.
> 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
atall. 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
theconnection 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().
Thats a good point, a successful connection does not inspect the errormessage.
I'll propose changes for these comments in the morning when coffee has been
had.
--
Daniel Gustafsson
