[PATCH] Add inotify support to imfile

This is the place for developers to discuss bugs, new features and everything else about code changes.

Google Ads


[PATCH] Add inotify support to imfile

Postby Martin Nordholts » Thu Feb 09, 2012 7:57 am

Hello,

I have written (and attached) a patch that adds inotify support to imfile and thus makes polling obsolete. Basic usage seems to work fine but I've not done extensive testing yet. I'm very interested in some feedback on this patch. How do you think it looks?

The design is as follows:
1. Add abstract "on file opened" and "on file closed" callbacks to strm_t
2. Add HAVE_SYS_INOTIFY_H #ifdefs in imfile, and if enabled, manage inotify watches on files using the above callbacks. Else do polling like before.

The use of inotify can be toggled with --enable/disable-inotify in the configure script.

Thanks for a great syslogger btw!

Best regards,
Martin
Attachments
Add-inotify-support-to-imfile.txt.zip
The patch, apply with git am patch.txt
(6.65 KiB) Downloaded 329 times
Martin Nordholts
New
 
Posts: 4
Joined: Thu Feb 09, 2012 7:34 am

Urgent Question?

  • Pulling out your Hair?
  • Wasting Time and Money?
  • Deadline Approaching?

Re: [PATCH] Add inotify support to imfile

Postby rgerhards » Thu Feb 09, 2012 8:37 am

Hi Martin,

thanks for the patch, this looks very interesting. I'll try to review it today, but can't guarantee that. One thing, though: we are currently re-licensing larger parts of rsyslog under the Apache Software License 2.0 (ASL 2.0) and imfile is among these. Do you agree to license your patch under ASL 2.0?

Thanks,
Rainer
rgerhards
Site Admin
 
Posts: 3806
Joined: Thu Feb 13, 2003 11:57 am

Re: [PATCH] Add inotify support to imfile

Postby Martin Nordholts » Thu Feb 09, 2012 4:39 pm

Hi

Take your time with reviewing, there's no rush. My spare time is limited so I won't be able to immediately react to your review comments anyway.

Regarding licensing, that's no problem at all. I hereby dual license all code I have contributed to rsyslog (the attached patch) and will contribute to rsyslog under GPLv2+ and Apache Licence 2.0 (http://www.apache.org/licenses/LICENSE-2.0.html).
Martin Nordholts
New
 
Posts: 4
Joined: Thu Feb 09, 2012 7:34 am

Re: [PATCH] Add inotify support to imfile

Postby JohnN » Wed Feb 22, 2012 8:00 pm

Hello,

I'm curious if you would compare your change with the work Nikolaidis did?

imfile-refactor-t10622.html

Thanks,

-john
JohnN
Frequent Poster
 
Posts: 116
Joined: Wed Jan 12, 2011 8:28 pm

Re: [PATCH] Add inotify support to imfile

Postby Martin Nordholts » Thu Feb 23, 2012 7:09 pm

Hi John,

Nikolaidis had a different goal, namely to refactor imfile and add more features than just inotify. This makes his patch more complex and risky and requires a bigger effort to review. I am a firm believer in incremental software development, so I decided to write a patch that does one thing and one thing only. That makes it easy to review and less risky to integrate upstreams. Besides, I wanted to have notify support now, and Nikolaidis thread seemed dead. Plus, I considered it a fun challenge to add inotify support to imfile.
Martin Nordholts
New
 
Posts: 4
Joined: Thu Feb 09, 2012 7:34 am

Re: [PATCH] Add inotify support to imfile

Postby JohnN » Mon Feb 27, 2012 6:41 am

Thanks for explaining.

-john
JohnN
Frequent Poster
 
Posts: 116
Joined: Wed Jan 12, 2011 8:28 pm

Re: [PATCH] Add inotify support to imfile

Postby rgerhards » Thu Mar 01, 2012 3:30 pm

Martin,

which version of rsyslog is this patch against? I am asking because I wanted to apply it, but it doesn't work with the current v5-devel or v5-stable or the current master version.

Thanks,
Rainer
rgerhards
Site Admin
 
Posts: 3806
Joined: Thu Feb 13, 2003 11:57 am

Re: [PATCH] Add inotify support to imfile

Postby Martin Nordholts » Sun Mar 04, 2012 4:06 pm

Here is the patch rebased on current v5-stable (536c9370f0 - Fix for "/run/systemd/journal/syslog" detection). When cherry-picking it to v5-devel or master, there's a trivial conflict in plugins/imfile/imfile.c's fileInfo_s, namely the introduction of 'multiSub'. Resolve it by keeping both 'multiSub' and my new struct members.
Attachments
0001-Add-inotify-support-to-imfile.zip
0001-Add-inotify-support-to-imfile.zip
(6.64 KiB) Downloaded 295 times
Martin Nordholts
New
 
Posts: 4
Joined: Thu Feb 09, 2012 7:34 am

Re: [PATCH] Add inotify support to imfile

Postby JohnN » Wed Apr 04, 2012 9:37 pm

Is there any plan to add inotify to imfile for Rsyslog 5.8.x? If so, what kind of ETA?

Thanks,

-john
JohnN
Frequent Poster
 
Posts: 116
Joined: Wed Jan 12, 2011 8:28 pm

Re: [PATCH] Add inotify support to imfile

Postby rgerhards » Tue Apr 10, 2012 9:32 am

Thanks for the reminder. There is so much going on right at the moment that I lost track of this thread... Anyhow, I have now merged the code into the new git v5-devel-imfile branch:

http://git.adiscon.com/?p=rsyslog.git;a ... vel-imfile

While I did some basic tests, I would appreciate some feedback from practice (John?) before I merge it into the fully official branch. As it is quite some new code, it won't make it into 5.8.x, but rather 5.9.x, which will become 5.10.0 (note that I plan to do this step in the not so distant future).

Rainer
rgerhards
Site Admin
 
Posts: 3806
Joined: Thu Feb 13, 2003 11:57 am

Re: [PATCH] Add inotify support to imfile

Postby JohnN » Tue Apr 10, 2012 4:06 pm

I don't mean to get off-topic, but is there a road-map somewhere? We are interested in this feature, but we are pretty committed to sticking with "stable". So it would be helpful for our planning purposes if we knew that 5.9 would become "stable" at some (estimated) point.

Thanks,

-john
JohnN
Frequent Poster
 
Posts: 116
Joined: Wed Jan 12, 2011 8:28 pm

Re: [PATCH] Add inotify support to imfile

Postby rgerhards » Tue Apr 10, 2012 4:20 pm

I've given up writing road maps, because things are so dynamic (except for paid custom projects with delivery date, of course). Anyhow, here is the overall philosophy:

http://blog.gerhards.net/2009/03/how-so ... table.html

It usually takes three month from a first beta to new stable. However, weighing all factors for v5-devel, I'd expect that it will be the new stable in roughly two month. There have not been many additions lately, so it somehow is in beta state for quite a while already, which will facilitate its promotion. I expect that inotify was probably that last major change for v5 (but never say never...).

HTH
Rainer
rgerhards
Site Admin
 
Posts: 3806
Joined: Thu Feb 13, 2003 11:57 am

Re: [PATCH] Add inotify support to imfile

Postby JohnN » Tue Jul 17, 2012 1:10 am

Just checking in for a status update on inotify support to imfile for "stable" (5.8.x).

Thanks,

-john
JohnN
Frequent Poster
 
Posts: 116
Joined: Wed Jan 12, 2011 8:28 pm

Google Ads



Return to Developer's Corner

Who is online

Users browsing this forum: No registered users and 1 guest

cron