> On 26 Jun 2025, at 22:49, Peter Eisentraut <pe...@eisentraut.org> wrote: > > On 03.04.25 17:51, Daniel Gustafsson wrote: >>> On 1 Apr 2025, at 22:22, Daniel Gustafsson <dan...@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 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(). 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
