[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


Review Part II

lwatch/watch_database.c:
sqlite3_result_string() - typo in the default case. "unkown" -> "unknown"

table_exists() - You're looping through results to see if the table
exists, but the SELECT query suggests that you should only be expecting
a single result. If you get more than one result back, shouldn't that
imply that the database has been corrupted and be treated as an error?

populate_path_info() - misuse of strncpy(). Not ensuring null
termination. You do this in many other places, so I will not repeat this
complaint again.
It would be prudent to have the positions of the values in the statement
be #defines instead of magic numbers, to make it easier to change later
if you add or remove columns.

path_info_flags_string() - Same snprintf() misuse as I mentioned in my
earlier review. Repeated in several other functions, so I will not
repeat myself.


util.c:
pathname_table_string() - You are misusing realloc(). If realloc()
returns NULL, you have leaked the original memory for tbl_string.

regexp.c:
Please prefer using the fully expanded error string PCRE_ERROR_* (as you
do with the SQLite error strings in watch_database.c)

inotify_watch.c:
I don't see any additional problems other than those indicated above.

This concludes my review of this iteration of the patch.

- -- 
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/

iEYEARECAAYFAkpotnUACgkQeiVVYja6o6MImgCfZf3qaMt+8wugE+WQTl5lKzYV
f3IAnjzMmsLz+g0s8iwH8NhexE6yoeya
=Utcx
-----END PGP SIGNATURE-----