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

[John Dennis]

On 07/22/2009 03:05 PM, Stephen Gallagher wrote:
> 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).

There is no concept of per user, this will morph into a system daemon 
thus there is no need for any of the above.

The use of of /tmp/lwatch.log is just pre-production staging, final 
production code will move it to a configured logging location (default 
/var/log/lwatch.log)

> DEBUG messages should be controlled by a commandline parameter.

of course :-)

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

good point and I am aware of it.

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

O.K., but note some of the existing Makefile.am's don't do this.

> Creation of /var/lib/lwatch needs to be part of 'make install' (e.g.
> create an install-exec-hook rule)

FWIW I think the location of the sqlite database should be a (yet to be 
created) config parameter. I do realize things like install rules and 
configuration parameters are absent, it's pre-production, but I will add 
them now rather than later.

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

yes, that's on the to-do list.

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

It does check:

     /* If the device or inode is different it must be a different file */
     if ((stat_info->st_dev != path_info->dev) || (stat_info->st_ino != 
path_info->inode)) {
         result = FILE_REPLACED;
     }


Thank you for your review Stephen, I'll push out a updated patch shortly 
to address your concerns.

-- 
John Dennis <jdennis at redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/