Bug #180

logrotate -> parse wrong file

Added by msk 9 months ago. Updated 3 months ago.

Status:New Start date:08/11/2011
Priority:Normal Due date:
Assignee:- % Done:

0%
Category:-
Target version:-

Description

If something like logrotate moves the games.log file, that file will continue to be used until the end of the current map, then a new games.log will be created. tremadminbot does not detect this, meaning that it will keep trying to parse an old games.log file forever.

The simplest solution is probably to go by file inode: at eof, if there is no current game (i.e., after ShutdownGame, before InitGame), compare the inode of the file named games.log with the (saved) inode of the file being parsed; if they are different, close the current file handle and open the new games.log (re-enabling startupBacklog just in case).

An alternative is to use inotify to find out when the file is moved, then reopen at ShutdownGame.

gameslogmove.patch - Account for games.log moving (3.5 kB) msk, 08/11/2011 07:59 pm

gameslogmove.patch - close FILE first (combined) (3.4 kB) msk, 08/13/2011 10:09 pm

smarterhup.patch - Smarter HUP handler (this obsoletes the other patches) (7.9 kB) msk, 08/15/2011 01:39 am

smarterhup-REAL.patch - Fixed smartedhup.patch (9.8 kB) msk, 08/20/2011 09:40 pm

smarterhup-REAL_1.patch - Add device test (apply after smarterhup-REAL.patch) (1007 Bytes) msk, 02/05/2012 01:44 pm

History

#1
Updated by msk 9 months ago

This patch makes the following changes
  • Change main regex so it matches ShutdownGame (which does not include a trailing space)
  • Move the code that handles opening the log and finding the last InitGame to a new function
  • Keep track of InitGame/ShutdownGame
  • At eof, if ShutdownGame has been seen since the last InitGame and the log file does not exist or has a different inode, try reopening the log file (giving up after 3 tries; once per second)

#2
Updated by Lakitu7 9 months ago

Note for me to fix when I get around to merging:

16:21 <Undeference> though actually the logrotate one doesn't close the file handle before
reopening, which might result in a warning (but is completely harmless)

#3
Updated by msk 9 months ago

Lakitu7 wrote:

Note for me to fix when I get around to merging:

I thought that was going to be on friday

#4
Updated by Lakitu7 9 months ago

I applied this patch and then (partly, initially) tested it by restarting the server while tremAdminBot was running. While it seems to successfully reopen games.log, it crashes immediately upon receiving a command. My immediate guess is that this same close/reopen behavior needs to be applied to com_pipefile.

#5
Updated by msk 9 months ago

The patch does not account for tremded being restarted, only the log file moving

#6
Updated by msk 9 months ago

  • Move more initialization code to separate functions
  • Use SIGHUP handler for initialization (it should now be safe to change things like $sendMethod while running)
  • Before writing to a fifo, attach a (temporary) SIGPIPE handler to try reopening the fifo (in case tremded restarted), with a hack to prevent dropping the message

$dbfile probably should not be changed while tremAdminBot.pl is running since some variables are expected to correspond to values in the db (e.g., userID) and it would require a lot more work to handle properly. But the db will be (safely) reopened when SIGHUP is received anyway.

#7
Updated by Lakitu7 9 months ago

I get an instacrash with this applied in what I think is a pretty normal, vanilla test setup. Loads fine without the patch.

patching file tremAdminBot.pl
Hunk #5 succeeded at 312 (offset 7 lines).
Hunk #6 succeeded at 415 (offset 7 lines).
Hunk #7 succeeded at 1042 (offset 3 lines).
$ ./tremAdminBot.pl
...
For details, see gpl.txt
-------------------------------------------------------------------------------------------
seek() on unopened filehandle FILE at ./tremAdminBot.pl line 411.
Error: seek fail at ./tremAdminBot.pl line 411.
seek fail at ./tremAdminBot.pl line 411.
$

#8
Updated by msk 9 months ago

smarterhup.patch was missing several hunks, which doesn't make sense since I'm pretty sure I tested it in detached head mode before uploading. Anyway, here is the fixed patch with the missing hunks and updated line numbers. And I tested it and it works (all I did is cherry pick the correct revisions and fix one conflict related to Geo::IP::PurePerl still being used upstream).

#9
Updated by msk 3 months ago

Checking inode alone is not sufficient

Also available in: Atom PDF