<html>
  <head>
    <meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    <br>
    <div class="moz-cite-prefix">On 08/26/2014 11:37 AM, Jakub Hrozek
      wrote:<br>
    </div>
    <blockquote cite="mid:20140826093736.GQ10132@hendrix.redhat.com"
      type="cite">
      <pre wrap="">On Mon, Aug 25, 2014 at 08:18:15PM +0200, Pavel Reichl wrote:
</pre>
      <blockquote type="cite">
        <pre wrap="">
On 08/25/2014 06:21 PM, Jakub Hrozek wrote:
</pre>
        <blockquote type="cite">
          <pre wrap="">On Mon, Aug 25, 2014 at 05:23:44PM +0200, Pavel Reichl wrote:
</pre>
          <blockquote type="cite">
            <pre wrap="">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.
</pre>
          </blockquote>
          <pre wrap="">That's a fair comment. Would you prefer to split the for loop into one
function or two?
</pre>
        </blockquote>
        <pre wrap="">two, but I can live with one :-)
</pre>
      </blockquote>
      <pre wrap="">
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.
</pre>
      <br>
    </blockquote>
    <br>
    After applying the patches I can't replicate the problem any
    longer.  <br>
    <br>
    Thanks for the code style change I think it's better now. <br>
    <br>
    Just 2 little details, please see inline, but I'm willing to ACK
    patches as are now.<br>
    <br>
    <blockquote type="cite">+static bool has_member(struct
      ldb_message_element *member_el,<br>
      +                       char *member)<br>
      +{<br>
      +    struct ldb_val val;<br>
      +<br>
      +    val.data = (uint8_t *) member;<br>
      +    val.length = strlen(member);<br>
      +<br>
      +    /* This is bad complexity, but the this loop should only be
      invoked in<br>
      +     * the very rare scenario of AD POSIX group that is primary
      group of<br>
      +     * some users but has user member attributes at the same time<br>
      +     */<br>
      +    if (ldb_msg_find_val(member_el, &amp;val)) {<br>
    </blockquote>
    <br>
    ldb_msg_find_val returns pointer so the if condition should look
    like:<br>
    <br>
    +    if (ldb_msg_find_val(member_el, &amp;val) != NULL) {<br>
    <br>
    right?<br>
    <br>
    <br>
    <blockquote type="cite">+        return true;<br>
      +    }<br>
      +<br>
      +    return false;<br>
      +}<br>
      +<br>
       static void link_pgroup_members(struct sysdb_attrs *group_attrs,<br>
                                       struct ldb_message_element
      *member_el,<br>
                                       char **userdns,<br>
                                       size_t nuserdns)<br>
       {<br>
      -    int i;<br>
      +    int i, j;<br>
       <br>
      +    j = 0;<br>
           for (i=0; i &lt; nuserdns; i++) {<br>
      -        member_el-&gt;values[member_el-&gt;num_values + i].data =
      (uint8_t *) \<br>
      -                                         
      talloc_steal(group_attrs, userdns[i]);<br>
      -        member_el-&gt;values[member_el-&gt;num_values + i].length
      = strlen(userdns[i]);<br>
      +        if (has_member(member_el, userdns[i]) == true) {<br>
    </blockquote>
    <br>
    has_member returns boolean so the '== true' is not needed, right?<br>
    <br>
    <blockquote type="cite">+            DEBUG(SSSDBG_TRACE_INTERNAL,<br>
      +                  "Member %s already included, skipping\n",
      userdns[i]);<br>
      +            continue;<br>
      +        }<br>
      +<br>
      +        member_el-&gt;values[member_el-&gt;num_values + j].data =
      (uint8_t *) \<br>
      +                                        
      talloc_steal(group_attrs, userdns[i]);<br>
      +        member_el-&gt;values[member_el-&gt;num_values + j].length
      = \<br>
      +                                         strlen(userdns[i]);<br>
      +        j++;<br>
           }<br>
      -    member_el-&gt;num_values += nuserdns;<br>
      +    member_el-&gt;num_values += j;<br>
       }</blockquote>
    <br>
    <blockquote cite="mid:20140826093736.GQ10132@hendrix.redhat.com"
      type="cite">
      <fieldset class="mimeAttachmentHeader"></fieldset>
      <br>
      <pre wrap="">_______________________________________________
sssd-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:sssd-devel@lists.fedorahosted.org">sssd-devel@lists.fedorahosted.org</a>
<a class="moz-txt-link-freetext" href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a>
</pre>
    </blockquote>
    <br>
  </body>
</html>