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

[John Dennis]

On 07/23/2009 09:01 PM, Dmitri Pal wrote:
> 1) There is generally a lack of comments in the code. it is very hard to
> read.
> IMO the lack of comments is a style issue rather then maturity of the
> code issue.

 > 13) I was amazed how well the path_util interface is defined and
 > documented .

As you observe I generally document my code well, you can see similar 
good documentation the C++ logger I just submitted as well. FWIW I 
usually document once the code stablizes. To me this is more an issue of 
being asked to submit the code for review before I was ready to 
"publish" it.

> 2) In many places the spaces are missing around commas, "-", "+".

Will fix in next patch.

> 3) Heavy use of the complex constructs:

O.K. I'll break these up as I encounter them.

>   4) In multiple places there is just one line of code inside the  {}
> after if for example:

Our coding standard recommends no braces when there is only a single 
expression. However I strongly disagree with this, in fact many coding 
standard recommend exactly the opposite, to always use braces! Why? 
Because I've seen plenty examples of bugs introduced when a programmer 
added a new expression to the conditional and totally broke the program 
logic. Usually these mistakes aren't caught until much later when things 
mysterously fail. I consider the use of braces a simple and effective 
example of defensive programming.

> 7) It should be a practice of using thread safe versions of system
> functions

  Just good defensive programming practice.

O.K. I'll switch over as time allows. FWIW the code wasn't designed to 
be thread safe, I never imagined it being run in seperate threads. But 
perhaps that should be one of our coding standards, that all code needs 
to be thread safe irrespective of it's intended usage. I'm aware of some 
other areas which are not thread safe at the moment, it was always my 
intention to clean those up anyway.


> 8) A lot of static global data. It would be beneficial to combine these
> things together in some kind of context structure and pass it around.

Actually it's not static, but you're right a number of the major data 
structures are singletons. At the moment I can't think of a reason why 
we would need multiple independent copies of the state, but perhaps down 
the road such a need might come up and having them hung off of a context 
would be necessary in that case. It's not needed now, but if it's not 
too difficult I'll gather them up in a context.

> Even if you keep it static for now it would be easier to refine code
> later. Currently too many low level functions rely the global data
> so it would require a significant refactoring effort to get rid of those
> down the road.
>
> 9) SSSD and Common and Audit all three use tree different ways of
> tracing/debugging things.
> Shouldn't we get to some consistency there?

yes, consistency would be better, the audit server adds a 4th (but in 
the case of the audit server I believe it's logging must be independent 
of ELAPI etc. because otherwise it creates a vicious cycle).

I would have used ELAPI but it wasn't ready when the code was written.

> It would be better if the log_msg be a macro from the very beginning.

It's a macro now :-)

> 11) From our design discussions I remember that  you mentioned that
> there was an issue with recursive directories and monitoring nested
> paths or something like this.
> It seems that find_existing_directory_ancestor() has to do with this issue.
> I was expecting to see some extended comment about this somewhere around
> create_watch() function.

Yup, as indicated above, more extensive documentation is on the to-do list.

> This is all for now.

Thank you for your review and attention. I'm preparing a new patch. I've 
got all of Stephen's issues fixed and I'll move on now to folding in 
your suggestions, the patch will follow.

-- 
John Dennis <jdennis at redhat.com>

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