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
>From 37aff8e90529e08d08607372875c684530407fb0 Mon Sep 17 00:00:00 2001
From: Nikolai Kondrashov <[email protected]>
Date: Fri, 15 Jul 2016 13:13:52 +0300
Subject: [PATCH] Add and correct some code documentation
---
src/responder/common/responder.h | 21 ++++++++++++---
src/responder/common/responder_packet.c | 45 +++++++++++++++++++--------------
src/responder/nss/nsssrv_cmd.c | 32 ++++++++++++++++++++++-
src/responder/nss/nsssrv_private.h | 26 +++++++++++--------
4 files changed, 90 insertions(+), 34 deletions(-)
diff --git a/src/responder/common/responder.h b/src/responder/common/responder.h
index 335b313..6a7ba2b 100644
--- a/src/responder/common/responder.h
+++ b/src/responder/common/responder.h
@@ -88,6 +88,7 @@ struct be_conn {
struct sbus_connection *conn;
};
+/** Responder context */
struct resp_ctx {
struct tevent_context *ev;
struct tevent_fd *lfde;
@@ -123,16 +124,17 @@ struct resp_ctx {
uint32_t cache_req_num;
- void *pvt_ctx;
+ void *pvt_ctx; /**< Responder-private context */
bool shutting_down;
};
struct cli_creds;
+/** Client context */
struct cli_ctx {
- struct tevent_context *ev;
- struct resp_ctx *rctx;
+ struct tevent_context *ev; /**< The tevent context to add events to */
+ struct resp_ctx *rctx; /**< Responder context */
int cfd;
struct tevent_fd *cfde;
tevent_fd_handler_t cfd_handler;
@@ -141,7 +143,7 @@ struct cli_ctx {
struct cli_creds *creds;
- void *protocol_ctx;
+ void *protocol_ctx; /**< Protocol context */
void *state_ctx;
struct tevent_timer *idle;
@@ -327,6 +329,17 @@ void idle_handler(struct tevent_context *ev,
#define GET_DOMAINS_DEFAULT_TIMEOUT 60
+/**
+ * Discover trusted domains and store them as sssd's subdomains.
+ *
+ * @param mem_ctx The memory context to allocate request with.
+ * @param rctx Responder context to work with.
+ * @param force Ignore timeouts and do request now, if true,
+ * observe timeouts, if false.
+ * @param hint
+ *
+ * @return Created event, or NULL if event creation failed.
+ */
struct tevent_req *sss_dp_get_domains_send(TALLOC_CTX *mem_ctx,
struct resp_ctx *rctx,
bool force,
diff --git a/src/responder/common/responder_packet.c b/src/responder/common/responder_packet.c
index 4f5e110..97f93b0 100644
--- a/src/responder/common/responder_packet.c
+++ b/src/responder/common/responder_packet.c
@@ -31,21 +31,22 @@
#define SSSSRV_PACKET_MEM_SIZE 512
struct sss_packet {
- size_t memsize;
-
- /* Structure of the buffer:
- * Bytes Content
- * ---------------------------------
- * 0-15 packet header
- * 0-3 packet length (uint32_t)
- * 4-7 command type (uint32_t)
- * 8-11 status (uint32_t)
- * 12-15 reserved
- * 16+ packet body */
- uint8_t *buffer;
-
- /* io pointer */
- size_t iop;
+ size_t memsize; /**< Packet buffer size */
+
+ uint8_t *buffer; /**< Packet buffer with the following structure:
+ * <pre>
+ * Bytes Content
+ * ---------------------------------
+ * 0-15 packet header
+ * 0-3 packet length (uint32_t)
+ * 4-7 command type (uint32_t)
+ * 8-11 status (uint32_t)
+ * 12-15 reserved
+ * 16+ packet body
+ * </pre>
+ */
+
+ size_t iop; /**< Current I/O pointer */
};
/* Offsets to data in sss_packet's buffer */
@@ -59,11 +60,17 @@ static void sss_packet_set_cmd(struct sss_packet *packet,
enum sss_cli_command cmd);
static uint32_t sss_packet_get_len(struct sss_packet *packet);
-/*
- * Allocate a new packet structure
+/**
+ * Allocate and initialize an empty packet.
+ *
+ * @param mem_ctx Context to allocate the packet with.
+ * @param size Packet body size.
+ * If zero, SSSSRV_PACKET_MEM_SIZE is used instead.
+ * @param rpacket Location for the created packet pointer, cannot be NULL.
*
- * - if size is defined use it otherwise the default packet will be
- * SSSSRV_PACKET_MEM_SIZE bytes.
+ * @return Error code:
+ * ENOMEM - memory allocation failed,
+ * EOK - packet created successfully.
*/
int sss_packet_new(TALLOC_CTX *mem_ctx, size_t size,
enum sss_cli_command cmd,
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index 1ae1796..f0291cc 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -234,6 +234,18 @@ static const char *get_homedir_override(TALLOC_CTX *mem_ctx,
dom->case_preserve, homedir_ctx);
}
+/**
+ * Retrieve a user's overriden shell value from an LDB message, according to
+ * global and user's domain-specific configuration.
+ *
+ * @param mem_ctx Context to allocate returned value with.
+ * @param msg The LDB message to retrieve user info from.
+ * @param nctx NSS context to operate within.
+ * @param dom Domain the user belongs to.
+ *
+ * @return User shell allocated with mem_ctx, or NULL on allocation error,
+ * or unspecified shell.
+ */
static const char *get_shell_override(TALLOC_CTX *mem_ctx,
struct ldb_message *msg,
struct nss_ctx *nctx,
@@ -400,6 +412,24 @@ done:
return ret;
}
+/**
+ * Transfer user information from LDB messages to a packet and NSS memory
+ * cache.
+ *
+ * @param packet The packet to transfer user information to.
+ * @param dom Domain the users belong to.
+ * @param nctx NSS context the operation is carried within.
+ * @param filter_users True if users present in negative cache should be
+ * skipped, false otherwise.
+ * @param pw_mmap_cache True if unskipped users should be stored in mmap
+ * cache referenced by nctx, if it's present.
+ * @param msgs Array of LDB messages containing user information to
+ * be transferred.
+ * @param count Number of LDB messages in msgs array to transfer user
+ * information from.
+ *
+ * @return Status code.
+ */
static int fill_pwent(struct sss_packet *packet,
struct sss_domain_info *dom,
struct nss_ctx *nctx,
@@ -781,7 +811,7 @@ errno_t check_cache(struct nss_dom_ctx *dctx,
return ENOENT;
}
- /* In case of local view we have to always contant DP with the original
+ /* In case of local view we have to always contact DP with the original
* name or id. */
get_dp_name_and_id(dctx->cmdctx, dctx->domain, req_type, opt_name, opt_id,
&name, &id);
diff --git a/src/responder/nss/nsssrv_private.h b/src/responder/nss/nsssrv_private.h
index 79c7b72..fb78d38 100644
--- a/src/responder/nss/nsssrv_private.h
+++ b/src/responder/nss/nsssrv_private.h
@@ -40,17 +40,21 @@ struct nss_state_ctx {
int netgrent_cur;
};
+/** NSS command execution context */
struct nss_cmd_ctx {
- struct cli_ctx *cctx;
- enum sss_cli_command cmd;
- char *name;
- const char *normalized_name;
- bool name_is_upn;
- uint32_t id;
+ struct cli_ctx *cctx; /**< Client context */
+ enum sss_cli_command cmd; /**< Command being executed */
+ char *name; /**< Name of the entry being requested */
+ const char *normalized_name; /**< Normalized name of the entry being requested */
+ bool name_is_upn; /**< Names above are Kerberos UPNs */
+ uint32_t id; /**< ID of the entry being looked up */
char *secid;
bool immediate;
- bool check_next;
+ bool check_next; /**< False if only the domain pointed to
+ by the domain context should be
+ looked up, true if following linked
+ domains should also be looked up */
bool enum_cached;
int saved_dom_idx;
@@ -77,13 +81,15 @@ struct getent_ctx {
};
struct nss_dom_ctx {
- struct nss_cmd_ctx *cmdctx;
- struct sss_domain_info *domain;
+ struct nss_cmd_ctx *cmdctx; /**< Parent command context */
+ struct sss_domain_info *domain; /**< Domain to start lookup from */
/* For a case when we are discovering subdomains */
const char *rawname;
- bool check_provider;
+ bool check_provider; /**< True if the current domain's
+ cache needs refresh from the
+ provider, false otherwise */
/* cache results */
struct ldb_result *res;
--
2.8.1
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]
