On Thu, Sep 16, 2021 at 05:27:20PM -0400, Sehrope Sarkuni wrote: > Attached three patches refactor the syslogger handling of file based > destinations a bit ... and then a lot. > > First patch adds a new constant to represent both file based destinations. > This should make it easier to ensure additional destinations are handled in > "For all file destinations..." situations (e.g. when we add the jsonlog > destination). > > Second patch refactors the file descriptor serialization and re-opening in > EXEC_BACKEND forking. Previously the code was duplicated for both stderr > and csvlog. Again, this should simplify adding new destinations as they'd > just invoke the same helper. There's an existing comment about not handling > failed opens in syslogger_parseArgs(...) and this patch doesn't fix that, > but it does provide a single location to do so in the future. > > The third patch adds a new internal (to syslogger.c) structure, > FileLogDestination, for file based log destinations and modifies the > existing handling for syslogFile and csvlogFile to defer to a bunch of > helper functions. It also renames "syslogFile" to "stderr_file_info". > Working through this patch, it was initially confusing that the stderr log > file was named "syslogFile", the C file is named syslogger.c, and there's > an entirely separate syslog (the message logging API) destination. I think > this clears that up a bit. > > The patches pass check-world on Linux. I haven't tested it on Windows but > it does pass check-world with EXEC_BACKEND defined to try out the forking > code paths. Definitely needs some review to ensure it's functionally > correct for the different log files.
Compilation with EXEC_BACKEND on Linux should work, so no need to bother with Windows when it comes to that. There is a buildfarm machine testing this configuration, for example. > I haven't tried splitting the csvlog code out or adding the new jsonlog > atop this yet. There's enough changes here that I'd like to get this > settled first. Makes sense. I am not really impressed by 0001 and the introduction of LOG_DESTINATIONS_WITH_FILES. So I would leave that out and keep the list of destinations listed instead. And we are talking about two places here, only within syslogger.c. 0002 looks like an improvement, but I think that 0003 makes the readability of the code worse with the introduction of eight static routines to handle all those cases. file_log_dest_should_rotate_for_size(), file_log_dest_close(), file_log_dest_check_rotate_for_size(), get_syslogger_file() don't bring major improvements. On the contrary, file_log_dest_write_metadata and file_log_dest_rotate seem to reduce a bit the noise. -- Michael
signature.asc
Description: PGP signature
