[SSSD] [PATCH] The new memberof plugin

Simo Sorce simo at redhat.com
Thu Sep 15 14:43:25 UTC 2011


On Tue, 2011-08-30 at 17:54 +0200, Jan Zelený wrote:
> Attached patch rewrites almost entire memberof plugin. It heavily utilizes 
> hash tables instead of lists and arrays and it introduces concept of reference 
> counting which should heavily optimize all operations when no loops are 
> present in the user/group tree.
> 
> I tested the patch by running sysdb test suite, all tests passed. I also 
> attach a document where basic concepts of the plugin are explained.
> 
> Please note that the patch is functional, although I don't consider it ready. 
> I just want to get pre-ACK or some comments about the patch design. I have yet 
> to implement the recompute task, I will work on that later.

Jan,
yesterday I an Stephen got together to start a review of the patch. Due
to the complexity we tried to go thorugh the code together to make sure
we understand what is going on.

Also due to the complexity and need to refresh memory we were only be
able to go through most of the "add" code only, we were not able to
review the update or delete code paths yet.

Attached find a diff file with comments in C++ style (so that it is
clear they all need to be resolved before the patch can be considered
ok, as they show as a big red line in our editors) that you can
integrate in your tree and check one by one.

There are some questions in there, but in general we noticed a very
annoying lack of comments in the code itself. The accompaning PDF is
nice and all, but unless the logic is embedded and explained in the code
(like it was with the previous code) it will be ablocker and will make
review more difficult.

That said, so far so good, although we need to more carefully review the
refcounting stuff, we got to it late and only marginally reviewed it.
Need more time to fully grasp all details. 

HTH,
Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
-------------- next part --------------
diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c
index 8d70967..c972fbb 100644
--- a/src/ldb_modules/memberof.c
+++ b/src/ldb_modules/memberof.c
@@ -1229,6 +1229,7 @@ static int mbof_update(struct mbof_update_ctx *update_ctx)
     }
 
     ldb = ldb_module_get_ctx(update_ctx->ctx->module);
+//REV: the name child does not convey enough context, perhaps use member ? See also previous comment about the term 'children'
     child = update_ctx->children[update_ctx->cur_child];
     child_name = ldb_msg_find_attr_as_string(child, DB_NAME, NULL);
     if (child_name == NULL) {
@@ -1446,6 +1447,8 @@ static int mbof_update_cb(struct ldb_request *req,
     return LDB_SUCCESS;
 }
 
+//REV: subtree is a bit confusing name, Stephen suggests membertree
+
 static int mbof_subtree_get_cb(struct ldb_request *req, struct ldb_reply *ares);
 
 static int mbof_subtree_get(struct mbof_ctx *ctx,
@@ -1629,6 +1632,8 @@ static int memberof_add(struct ldb_module *module, struct ldb_request *req)
     }
     add_ctx->msg_dn = add_ctx->msg->dn;
 
+//REV: Check member earlier and call ldb_next_request() if not foudn
+
     /* continue with normal ops if there are no members */
     el = ldb_msg_find_element(add_ctx->msg, DB_MEMBER);
     if (!el) {
@@ -1690,11 +1695,13 @@ static int mbof_add_cb(struct ldb_request *req,
                                    LDB_SUCCESS);
         }
 
+//REV: Conditional never possible! cur_member always == 0 here
         if (add_ctx->cur_member >= add_ctx->member_el->num_values) {
             /* no more operations */
             if (add_ctx->missing) {
                 ret = mbof_add_delete_missing(add_ctx);
             }
+//REV: Add comment, this adds memberuid attributes
             else if (add_ctx->muops) {
                 ret = mbof_add_process_muop(add_ctx);
             }
@@ -1705,6 +1712,7 @@ static int mbof_add_cb(struct ldb_request *req,
                                        LDB_SUCCESS);
             }
         } else {
+//REV: Probably no reason for conditional here, mobof_add_cb called only from one place
             if (add_ctx->cur_member == 0) {
                 ctx->ret_ctrls = talloc_steal(ctx, ares->controls);
                 ctx->ret_resp = talloc_steal(ctx, ares->response);
@@ -1745,6 +1753,7 @@ static int mbof_add_next_child(struct mbof_add_ctx *add_ctx)
     if (ldb_dn_compare(valdn, add_ctx->msg_dn) == 0) {
         ldb_debug(ldb, LDB_DEBUG_ERROR,
                   "Adding self as member is not permitted! Skipping");
+//REV: We said we would skip this!
         ret = LDB_ERR_OPERATIONS_ERROR;
         goto fail;
     }
@@ -1764,6 +1773,9 @@ fail:
     return ret;
 }
 
+//REV: C++ COMMENTS ?
+
+//REV: replace evrywhere (add operation) children with fullmembership
 static int mbof_add_next_child_cb(void *pvt, struct ldb_message **children,
                                   int num)
 {
@@ -1784,6 +1796,8 @@ static int mbof_add_next_child_cb(void *pvt, struct ldb_message **children,
         ldb_debug(ldb, LDB_DEBUG_TRACE, "Entry not found (%s)",
                 ldb_dn_get_linearized(valdn));
 
+//REV: Why do we do this ? We should probably fail on missing members.
+
         /* this target does not exists, save as missing */
         ret = mbof_add_missing_append(add_ctx, valdn);
         if (ret != LDB_SUCCESS) {
@@ -1796,6 +1810,7 @@ static int mbof_add_next_child_cb(void *pvt, struct ldb_message **children,
         add_ctx->children = children;
         add_ctx->num_children = num;
 
+//REV: pass children and num_children as arguments as they are cleared from add_ctx anyway
         ret = mbof_add_prepare_update(add_ctx, &update_ctx);
         if (ret != LDB_SUCCESS) {
             return ret;
@@ -1819,6 +1834,7 @@ static int mbof_add_next_child_cb(void *pvt, struct ldb_message **children,
     }
 }
 
+//REV: probably good idea to merge this function with the previous
 static int mbof_add_prepare_update(struct mbof_add_ctx *add_ctx,
                                    struct mbof_update_ctx **_update_ctx)
 {
@@ -1859,6 +1875,7 @@ static int mbof_add_prepare_update(struct mbof_add_ctx *add_ctx,
 
     key.type = HASH_KEY_STRING;
     value.type = HASH_VALUE_LONG;
+//REV: comment what is the following code doing please!
     if (add_ctx->follow_mod) {
         update_ctx->ancestors = add_ctx->follow_mod->ancestors;
     } else {
@@ -1919,6 +1936,7 @@ static int mbof_add_finish_child(void *pvt)
             ret = mbof_mod_finish(add_ctx->follow_mod);
         }
         else {
+//REV: use destructors please, no explicit cleanup unless necessary
             hash_destroy(add_ctx->ancestors);
             return ldb_module_done(ctx->req,
                     ctx->ret_ctrls,
@@ -2063,7 +2081,7 @@ static int mbof_add_delete_missing_cb(struct ldb_request *req,
     talloc_zfree(ares);
     return LDB_SUCCESS;
 }
-
+//REV: We should add memberuid attributes only to groups that have posixGroup objectclass,things like HBC groups do not need nor want it.
 /* add memberuid attributes to parent groups */
 static int mbof_add_process_muop(struct mbof_add_ctx *add_ctx)
 {


More information about the sssd-devel mailing list