[syslog-ng] log file size limit

[Balazs Scheidler]

On Fri, 2011-08-19 at 12:01 +0200, Gergely Nagy wrote:
> Gergely Nagy <algernon at balabit.hu> writes:
> 
> > Balazs Scheidler <bazsi at balabit.hu> writes:
> >
> >> On Mon, 2011-07-25 at 20:46 +0200, Gergely Nagy wrote:
> >>> Sergei Zhirikov <sfzhi at yahoo.com> writes:
> >>> >>> I posted that patch in this list quite some time ago, but as far as I can tell, it went unnoticed...
> >>> >>
> >>> >> Can you please post it again? I'd love to have a look.
> >>> >>
> >>> >
> >>> > Attached.
> >>> 
> >>> Thank you, I will have a look hopefully within a day or two!
> >>> 
> >>> (Judging by the description, it sounds very useful & promising to me!)
> >>> 
> >>
> >> any news on this?
> >
> > Not yet. Got caught up in DebConf stuff, and still not recovered. I'll
> > have a go at it this coming friday.
> 
> Update: I'm halfway through. Applied the patch, made it compile, figured
> out why it doesn't work (it modified LogTransportPlain to have a
> callback, but nowadays we're using LogProtoFileWriter to write to files,
> so the callback never gets called.
> 
> My first idea would be to port the patch to extend LogProtoFileWriter
> instead of LogTransportPlain, but I'm not done figuring out how that
> part of the code works, yet.
> 
> In any case, the patch itself looks good, there's only one thing I will
> want to change after porting it to 3.3: I don't like how the renamed
> file has a hard-coded template. I'd much rather introduce a second,
> optional argument to size_limit(), which would be the template to rename
> the old file to (defaulting to something like the current template,
> which appends a timestamp and a random number).
> 
> However, adding such a template will need a bit more thinking, as I'll
> need to pass down the neccessary structures to the callback.
> 
> Or perhaps, the callback could be rewritten to not do the actual file
> reopening, just determine if we need to do that. And if so,
> log_proto_writer_flush() would bubble up, we'd catch it somewhere in
> affile, notice a flag the callback would set, and do the reopen, and
> retry.
> 
> This way we have easy access to the message, and the callback would be
> very lightweight.
> 
> I'll see what I can do about this.
> 

I've reviewed this code and I have some open issues, like:

1) the LogProtoFileWriter is only used by the file destination, no need
to use a separate constructor. I think it'd make sense to move the
FileWriter proto class into the file plugin.

2) I don't like the global option. For one I doubt that a global size
limit would ever really be used: if they are really multiple destination
files, they should probably have different size limits.

3) The sequence number of renaming should probably be persistent, as
reloads/restarts will trigger that counter to restart from 1. And I
don't really like to overload the $SEQNUM macro which is defined for a
completely different purpose.

Otherwise I do like the functionality, but I don't really have an answer
for these questions either. The code could probably be made nicer,
simply be not pretending that LogProtoFileWriter is generic code. It is
not, it belongs to the file plugin.

The SEQNUM stuff is a good idea (I really couldn't decipher it first,
but then I liked it), however it can be confusing.

Certainly I'd need some thinking time, but I thought I'd post these
concerns anyway, to see that I'm not just idling on integrating it.

-- 
Bazsi