On Thu, Sep 16, 2021 at 9:36 PM Michael Paquier <michael@paquier.xyz> wrote:
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.
I've taken that out for now. The idea was to simplify the additions when jsonlog is added but can circle back to that later if it makes sense.
0002 looks like an improvement,
Nice. That's left unchanged (renamed to 0001 now).
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.
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.