Hi, Tomas.
08.12.2024 01:06, Tomas Vondra wrote:
> Yeah, creating all those files seems rather unnecessary. But wouldn't it
> be easier to do the check in logfile_rotate_dest(), instead of for each
> call? I think something like this would do the trick:
>
> @@ -1292,6 +1292,9 @@ logfile_rotate_dest(bool time_based_rotation, int
> size_rotation_for,
> if (!time_based_rotation && (size_rotation_for & target_dest) == 0)
> return true;
>
> + if ((Log_destination & target_dest) == 0)
> + return true;
> +
> /* file extension depends on the destination type */
> if (target_dest == LOG_DESTINATION_STDERR)
> logFileExt = NULL;
Thanks for the comments, I agree that it is better to move the check to
logfile_rotate_dest().
> In fact, isn't it wrong to do the check outside? logfile_rotate_dest()
> is responsible for closing the log files too, and with the check outside
> we keep the first .log file open forever (lsof shows that).
>
> FWIW my fix has the same issue, but IMO that just means the logic needs
> to be more complex, but still in logfile_rotate_dest().
You can close the first file if you remove the second part of the
condition in
if (because it is stderr). I checked it in lsof and the first log file is
closing:
@@ -1274,8 +1274,7 @@ logfile_rotate_dest(bool time_based_rotation, int
size_rotation_for,
* is assumed to be always opened even if stderr is disabled in
* log_destination.
*/
- if ((Log_destination & target_dest) == 0 &&
- target_dest != LOG_DESTINATION_STDERR)
+ if ((Log_destination & target_dest) == 0)
{
if (*logFile != NULL)
fclose(*logFile);
But the comment above says that stderr should always remain open, even if
disabled in log_destination. Is it really necessary to do more complex
logic
in order to close this file? I'm sorry if I misunderstood something.
What do you think about this?
Best regards,
Arseny Kositsin.
