[Freeipa-devel] [PATCH] initial commit of log watcher (lwatch)

[Stephen Gallagher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/16/2009 04:49 PM, John Dennis wrote:
> This is a big patch, sorry, but there just isn't a realistic way to
> develop a major piece of code into a working state given our checkin
> policy which requires peer review for every change, it's just easier to
> develop elsewhere and submit the working result.
> 
> This is the basic framework for the client audit code, it produces an
> executable called lwatch. In it's default mode lwatch watches a set of
> (log) files. It understands how log files are rotated. When a log file
> reaches a modification threshold it's newly appended data is prepared
> for transport to the audit server. When the log file is rotated,
> created, renamed, etc. the appropriate actions take place.
> 
> lwatch maintains it's persistent state in a sqlite database. You can
> start and stop lwatch and it self synchronizes based on what is in the
> database and what it finds in the file system.
> 
> lwatch is also capable of listing every log under a directory root, this
> can be handy for initializing the set of log files to watch.
> 
> lwatch can also dump in a pretty format the current state of the SQL
> database.
> 
> lwatch is not completely finished, but it's been in reasonable working
> shape for a while now, but sitting in my own git repository, it's time
> to get into the project's repository. What it needs next is better
> configuration options and to link in the code for audit server
> transmission (coming soon).
> 
> Just one note about the patch, it includes a trivial one line fix to
> dhash.h which was missing a const declaration. I realize that should
> have been in a separate patch, but it got included here.
> 
> 
> ------------------------------------------------------------------------
> 
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

Nack. The following things must be corrected before this patch will be
accepted.

General:
The log file /tmp/lwatch.log is not usable by two users simultaneously.
Each user should have his/her own log file or there should be a
mechanism in place to log safely to the common log.
There is a similar problem with the sqlite database. The first user to
execute lwatch gains permissions on the database. No other users have
write access after that (except root).
DEBUG messages should be controlled by a commandline parameter.

/lwatch/configure.ac:
You need to have a configure test for sqlite3.

/lwatch/src/Makefile.am:
Don't use $(srcdir) as a path to built binaries. This doesn't support
parallel build trees. For linking dhash and path_utils, it needs to be
relative to $(builddir).
Creation of /var/lib/lwatch needs to be part of 'make install' (e.g.
create an install-exec-hook rule)

/lwatch/src/lwatch.c:
The return values from file_event_string should be localized.
log_msg() should be a macro. It's very ugly reading __FUNCTION__ all
over the code.
In process_file_rename() and many other places, you are using strncpy
unsafely. If the source string is equal or longer than the destination
string, strncpy() does not null-terminate. As a result, reading from it
could wander off the page. It's always wise to do strncpy(dest, orig,
sizeof(dest)-1);
dest[sizeof(dest)-1] = '\0';
lwatch_event_string() is using snprintf() unsafely. If the string (after
substitution) exceeds "buf_end-p", the function will only write
buf_end-p values, but it will return the TOTAL size that would have been
written if it had not been truncated. You need to test for that.



Things that aren't necessarily worth holding up the commit for:

I'd like to recommend that you switch to using tevent for processing
your mainloop and signal handlers. It's much safer for the latter than
just catching signals, as it will just queue the handler processing for
the next loop, instead of breaking execution.

compare_stat_info() makes an assumption that if the filesize is larger,
then it's been appended to. It's possible that a larger file has been
moved into place atop the older file. Perhaps you can check whether the
inode number has changed?


I have so far only reviewed lwatch.c. I will review other components
over the next day or two, but I figured I'd send back these comments first.
- -- 
Stephen Gallagher
RHCE 804006346421761

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkpnYuEACgkQeiVVYja6o6OOEACeIW5DTfraYeShxwMeRVb81Z63
hPwAnjr6jYd9/nMAnZ6qkN7SeTlkS6qG
=qL61
-----END PGP SIGNATURE-----