The sorry state of code documentation
by Nikolai Kondrashov
Hi everyone,
I've been acquainting myself with SSSD code the last few weeks. Particularly,
the NSS responder, and what it uses down the stack.
The SSSD code is sure complex and its asynchronous nature doesn't help.
However, what struck me, and frustrated me the most, as a new reader of the
code, was the meager amount of code documentation.
In the absence of documentation, a piece of logic, such as processing a
getpwnam request, requires traversing many functions, modules, and several
programs. Sure, you can guess what a function does, based on its name and
somewhat on arguments, but you can't be sure without reading it and the
functions it calls, and often the functions those call in turn. Same goes with
data structures: to understand them you need to read the code that creates and
operates them.
To the author code often seems obvious, but a new reader has a dozen
questions: which state the passed object should be in, how does the function
modify the object, where does it put its output, what else it affects, exactly
what string should be that, what status codes the function can return and what
will they mean, etc.
Documentation cements the author's intention in the mind of the reader,
freeing the memory and precious attention for understanding the code at hand,
as opposed to requiring going deeper and deeper into the underlying code,
where the reader eventually forgets why he or she is looking at it. Reading
such complicated code as can be seen in SSSD without documentation is an
exercise in frustration.
We often speak about cooperation and getting more external contributors.
However, we're not going to get many if they have such a mountain of code to
climb without our help. We ourselves will be (and are) struggling to
understand our own code, and will pay with lost time again and again when
trying to modify or extend it.
In short: it is NOT OK that SSSD code has no documentation.
Put bluntly, it is sloppy engineering. I'm sure we can do better.
If you have just a sentence describing each function and each of its
arguments, understanding the code becomes considerably easier. You don't have
to guess that much, and you don't have to verify your guesses and read so much
more. Function documentation would often make reading its code unnecessary.
A sentence for a data structure and each of its fields makes understanding the
code operating it much easier.
I suggest several measures which could be taken.
1. Require *every* new module/function/argument/structure/field to be
documented. It must be at least a sentence for each. A module or a bigger
data structure could warrant more. Describe module/function/structure
overall, and each of its declarations/arguments/return values/fields.
2. Next level, documentation tax: if you touch anything, document it. Modified
a module header? Add a short module description. Added or removed a field
in a structure? Add structure and field descriptions. Changed a function?
Document it.
3. Third level, documentation duty: document a whole existing module header of
your choosing, say once a month.
The format of documentation doesn't matter, as long as it's legible and agreed
upon. I suggest Doxygen, but it can be anything else. We don't have to
actually run the documentation extraction tool (such as "doxygen") on our code
and use perfectly correct syntax (although that could be the next step). What
matters is that documentation is there, it's uniform, and you don't have to
think about the format when writing it.
For example of how it can look see the attached patch where I tried to
document some things (likely misunderstanding some) as I read the code.
For more examples of what I mean see the tlog header files:
https://github.com/Scribery/tlog/tree/master/include/tlog
They are not the best example and there are many ways they can be improved
(tell me please!), but if SSSD had just that it would be already awesome.
Please tell me what you think, object, or suggest your own solutions!
Thank you.
Nick
7 years, 7 months
[PATCH SET] AD_PROVIDER: ad_enabled_domains
by Petr Cech
Hello list,
there is patch set attached resolving:
https://fedorahosted.org/sssd/ticket/2828
You can also see:
https://github.com/celestian/sssd/commits/subdomains_v2
This patch set is without tests yet.
I will send tests after Pavel B. refactor of AD provider.
I would like to remark couple of things:
1) Option ad_enabled_domains disabled by default.
2) We use labels master domain and root domain in code. Master domain
belongs to one that SSSD is connected to. Root domain means AD forest root.
3) There is a lot of different environment setup. So this option has
slightly different meaning in each of them.
4) There is secret feature if you forget fill in master domain to the
option it will be added automatically.
What is this option good for?
Let's imagine environment like:
A (forest root)
|
|-- S1 (SSSD client)
|
|-- B (AD client)
| |
| |-- S2 (SSSD client)
|
|-- C (AD client)
|
|-- D (AD client)
A ... forest root
B, C, D ... AD clients
S1 ... SSSD client connected to A
S2 ... SSSD client connected to B
We could write:
* on S1: ad_enabled_domain = A, B, C
and then S1 will not attempt to connect D.
* on S1: ad_enabled_domain = B, C
the same as above because A is automagically
filled in.
* on S2: ad_enabled_domain = A, C
and then B is filled in automatically
and S2 will not attempt to connect D.
* on S2: ad_enabled_domain = C
and then B is filled in automatically
and S2 will not attempt to connect D and A.
(I am wondering that it is possible but it
happened during manual testing.)
Regards
--
Petr^4 Čech
7 years, 7 months
[PATCH] IPA: Removing of confusing debug message
by Petr Cech
Hello list,
there is simple patch for [1].
I prepared two version:
a) the first is for changing the message,
b) the second is for removing.
I looked at the code -- I think it could be useful to know what type of
direction we have. But I agree that it could be really confusing if we
see 'trust direction not set'.
So I vote for removing.
Please do you have any other opinion?
[1] https://fedorahosted.org/sssd/ticket/3090
Regards
--
Petr^4 Čech
7 years, 7 months
[PATCH] sssctl: Consistent commands naming
by Michal Židek
Hi,
this patches makes the sssctl commands more similar to
ipa tool commands. I also think this pattern makes it
easier to remember the commands.
Note that in the future, there will be more user-*
group-* and netgroup-* commands (like seed for user,
list of all etc.)
Comments are welcome.
Michal
7 years, 7 months