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.