On 08/26/2014 11:37 AM, Jakub Hrozek wrote:
On Mon, Aug 25, 2014 at 08:18:15PM +0200, Pavel Reichl wrote:
On 08/25/2014 06:21 PM, Jakub Hrozek wrote:
On Mon, Aug 25, 2014 at 05:23:44PM +0200, Pavel Reichl wrote:
Hello,

I tested the patch and it fixes the problem described in the ticket.

I personally dislike the break and mainly the continue check - I believe
that people reading the code would benefit from encapsulating the inner loop
into function (return value would indicate what is happening and function
name is the best comment anyway) and same would go for the outer loop IMO.
That's a fair comment. Would you prefer to split the for loop into one
function or two?
two, but I can live with one :-)
OK, see the attached patches. Please note I wasn't able to test them
properly because my new AD test servers don't carry POSIX attributes
yet. I'll test the patches in a short while, in the meantime, feel free
to test in your environment and/or look at the code.


After applying the patches I can't replicate the problem any longer. 

Thanks for the code style change I think it's better now.

Just 2 little details, please see inline, but I'm willing to ACK patches as are now.

+static bool has_member(struct ldb_message_element *member_el,
+                       char *member)
+{
+    struct ldb_val val;
+
+    val.data = (uint8_t *) member;
+    val.length = strlen(member);
+
+    /* This is bad complexity, but the this loop should only be invoked in
+     * the very rare scenario of AD POSIX group that is primary group of
+     * some users but has user member attributes at the same time
+     */
+    if (ldb_msg_find_val(member_el, &val)) {

ldb_msg_find_val returns pointer so the if condition should look like:

+    if (ldb_msg_find_val(member_el, &val) != NULL) {

right?


+        return true;
+    }
+
+    return false;
+}
+
 static void link_pgroup_members(struct sysdb_attrs *group_attrs,
                                 struct ldb_message_element *member_el,
                                 char **userdns,
                                 size_t nuserdns)
 {
-    int i;
+    int i, j;
 
+    j = 0;
     for (i=0; i < nuserdns; i++) {
-        member_el->values[member_el->num_values + i].data = (uint8_t *) \
-                                          talloc_steal(group_attrs, userdns[i]);
-        member_el->values[member_el->num_values + i].length = strlen(userdns[i]);
+        if (has_member(member_el, userdns[i]) == true) {

has_member returns boolean so the '== true' is not needed, right?

+            DEBUG(SSSDBG_TRACE_INTERNAL,
+                  "Member %s already included, skipping\n", userdns[i]);
+            continue;
+        }
+
+        member_el->values[member_el->num_values + j].data = (uint8_t *) \
+                                         talloc_steal(group_attrs, userdns[i]);
+        member_el->values[member_el->num_values + j].length = \
+                                         strlen(userdns[i]);
+        j++;
     }
-    member_el->num_values += nuserdns;
+    member_el->num_values += j;
 }


_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel