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 <jden...@redhat.com>
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
