Re: [PATCH] Fixed creation of empty .log files during log rotation

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.