sssd-devel July 2016

sssd-devel@lists.fedorahosted.org

18 participants
103 discussions

The sorry state of code documentation
by Nikolai Kondrashov 10 Aug '16

10 Aug '16

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

4 13

[PATCH] sssctl: use internal API to remove files
by Pavel Březina 09 Aug '16

09 Aug '16

3 6

[PATCH] UTILS: Fixing duplication of pid file declaration
by Petr Cech 09 Aug '16

09 Aug '16

Hello list, attached patch fixes duplication of pid file declaration. I hope that the util/util.h is the right place for it. Another opinion are welcome. Regards -- Petr^4 Čech

3 6

[PATCH SET] AD_PROVIDER: ad_enabled_domains
by Petr Cech 09 Aug '16

09 Aug '16

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

4 14

[PATCH] sssctl: Generic help for cache-upgrade and config-check
by Michal Židek 09 Aug '16

09 Aug '16

Hi! Attached is patch for ticket: https://fedorahosted.org/sssd/ticket/3086 This patch applies on top of the patches from thread: [SSSD] [PATCH] sssctl: Consistent commands naming Michal

3 4

[PATCH] PROVIDERS: Default values in debug
by Petr Cech 09 Aug '16

09 Aug '16

Hi list, I was debug code very deeply and I found nitpick. There is simple patch attached. Regards -- Petr^4 Čech

3 10

[PATCH] sssctl: print a message when managing sssd
by Pavel Březina 05 Aug '16

05 Aug '16

3 5

[PATCH] PROVIDER: Conversion empty string from D-Bus to NULL
by Petr Cech 05 Aug '16

05 Aug '16

Hi list, there is patch for [1] attached. [1] https://fedorahosted.org/sssd/ticket/3084 If you prefer two commits--one for reverting and second with the patch--don't hesitate to tell me it. :-) Regards -- Petr^4 Čech

4 6

[PATCH] IPA: Removing of confusing debug message
by Petr Cech 05 Aug '16

05 Aug '16

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

4 7

[PATCH] sssctl: Consistent commands naming
by Michal Židek 05 Aug '16

05 Aug '16

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

5 32

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

sssd-devel July 2016