On Fri, Sep 17, 2021 at 04:36:57PM -0400, Sehrope Sarkuni wrote:
> It was helpful to split them out while working on the patch but I see your
> point upon re-reading through the result.
>
> Attached version (renamed to 002) adds only three static functions for
> checking rotation size, performing the actual rotation, and closing. Also
> one other to handle generating the logfile names with a suffix to handle
> the pfree-ing (wrapping logfile_open(...)).
>
> The rest of the changes happen in place using the new structures.
I have looked at that in details, and found that the introduction of
FileLogDestination makes the code harder to follow, and that the
introduction of the file extension, the destination name and the
expected target destination LOG_DESTINATION_* had a limited impact
because they are used in few places. The last two useful pieces are
the FILE* handle and the last file name for current_logfiles.
Attached are updated patches. The logic of 0001 to refactor the fd
fetch/save logic when forking the syslogger in EXEC_BACKEND builds is
unchanged. I have tweaked the patch with more comments and different
routine names though. Patch 0002 refactors the main point that
introduced FileLogDestination by refactoring the per-destination file
rotation, not forgetting the fact that the file last name and handle
for stderr can never be cleaned up even if LOG_DESTINATION_STDERR is
disabled. Grepping after LOG_DESTINATION_CSVLOG in the code tree, I'd
be fine to live with this level of abstraction as each per-destination
change are grouped with each other so they are hard to miss.
0001 is in a rather commitable shape, and I have made the code
consistent with HEAD. However, I think that its handling of
_get_osfhandle() is clunky for 64-bit compilations as long is 32b in
WIN32 but intptr_t is platform-dependent as it could be 32b or 64b, so
atoi() would overflow if the handle is larger than INT_MAX for 64b
builds:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/standard-types
This problem deserves a different thread.
It would be good for 0002 if an extra pair of eyes looks at it. While
on it, I have renamed the existing last_file_name to
last_sys_file_name in 0002 to make the naming more consistent with
syslogFile. It is independent of 0001, so it could be done first as
well.
--
Michael
