[Freeipa-devel] [PATCH] initial commit of audit server code

[Jakub Hrozek]

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

Hello John,

On 07/22/2009 07:54 PM, John Dennis wrote:
> In the spirit of small patches this is the most minimal patch possible
> to get any part of the audit server in. It contains the basic autotools
> framework and just 1 c++ component, the logger facility used by the
> audit server. Because it's minimal you can't do anything with it except
> review 2 source files and build the documentation for it.
> 

I would argue that the patch is too minimal..or maybe it seems like
ripped out of context. On one hand, it defines
bin_PROGRAMS=audit_server, on the other hand, there is no main() in
audit_server_SOURCES to build audit_server with. Also, why is
audit_server_LDADD commented out? At least -lboost_regex is needed for
the current patch. Anyway, I did not perform any testing aside from
compiling the code, I just read the code during the review.

* The patch introduces whitespace errors, which is by no means a reason
to nack the work, but perhaps it is something that can be remedied with
a simple editor setting (like let g:c_space_errors=1 for vim)?

* Not sure if including a license and/or copyright notice in source
files is a requirement for the project. Is it licensed in same way as
sssd is?

* Is there a unit test for the module? There are some code examples in
doxygen comment headers, maybe they could be the basis of tests. (Also a
more general question - if we write unit tests for C++ code, have we
agreed on some common framework, like check is used for C code in SSSD?)

* I did not understand why LogLevelTraits is declared as struct..I
thought the the reason for this is usually to get default-public
inheritance (and access, but that could be done with modifiers more
cleanly, IMHO), but no classes seem to inherit from LogLevelTraits
(yet?). Maybe it's worth a comment in the declaration?

* I also do not understand exactly how filtering by categories works,
the code in is_filtered_by category looks like it will return true on
first non-match, so the category must match all the patterns in
category_include. Is that correct?

Nitpicking:
* I spotted several typos..maybe worth fixing for future:
248: formated (formatted)
250: messgae (message)
333: to adopted (to be adopted)

* Only a matter of style, but I do not like how the instance.log() call
is duplicated in Logger::log(), I think it's more readable to do
something like:
- -------
if (perform_filtering &&
    is_filtered(msg.get_level(), msg.get_category()) {
       return;
}

Logger::instance().log(msg.format(prefix, unknown_string));
- -------

* Most methods begin with the opening curly brace on its own line,
Msg::insert() and some in class Logger do not. It would probably be nice
to stick to one style.

* Some reference parameters seem to be declared as "type& name" and some
as "type &name", not sure if it's following any convention.

> To build the doc you'll need doxygen installed, then:
> 
> cd doc
> make doc
> 
> Then open in your web browser doc/api/html/index.html

Thanks, I really like how thorough the code is documented. Does it make
sense to set PROJECT_NAME=audit_server or similar to Doxyfile? Also,
would it make sense to remove the api subdirectory if it already exists,
so refreshing documentation is easier?

   Jakub
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkpt7fkACgkQHsardTLnvCVtYwCgpDCy2AGNICJZx9I14meV1+jp
urEAnjJPS+EkhX8+gkU38ViJOrFt2Rpy
=JR0b
-----END PGP SIGNATURE-----