[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