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

 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 <jden...@redhat.com>

Looking to carve out IT costs?

Freeipa-devel mailing list

Reply via email to