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

[Stephen Gallagher]

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

On 07/22/2009 03:58 PM, John Dennis wrote:
> 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.
>

Can you point me at examples of this? I build exclusively in parallel
trees (mainly to catch cases where it fails) and the build system is
working correctly.

$(builddir) should be used wherever built files are needed, and
$(srcdir) should be used wherever the path needs to be relative to the
uncompiled sources.

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

Thanks. I agree that it should be a configure parameter. I was just
going to let that one slide for first commit.

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

Sorry missed that in my review.

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

Thanks for the replies. I'm still working on my review. Expect more
comments later today.

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

iEYEARECAAYFAkpofNgACgkQeiVVYja6o6OI9QCcC2yS/QY4IcI2qwJ2bm3jGoKS
DB0AnRZ1yOgA/IZoLncXItWpMmz8a4GJ
=KyHJ
-----END PGP SIGNATURE-----