[SSSD] [PATCH] LDAP: Don't add a user member twice when adding a primary group

Pavel Reichl preichl at redhat.com
Tue Aug 26 12:01:07 UTC 2014


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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140826/bc09c936/attachment.html>


More information about the sssd-devel mailing list