[SSSD] [PATCH] When ldap_group_nesting_level was reached, the LDAP provider, tried to link group members with groups outside nesting, limit.

Simo Sorce simo at redhat.com
Mon Aug 6 18:57:10 UTC 2012


On Mon, 2012-08-06 at 20:29 +0200, Michal Zidek wrote:
> Now, the groups outside nesting limit are skipped.
> The patch is attached.
> 
> https://fedorahosted.org/sssd/ticket/1194

Mnior nitpicks and one important question inline.

> From b16c02579bb94d0058cde0f890167cccb47b3899 Mon Sep 17 00:00:00 2001
> From: Michal Zidek <mzidek at redhat.com>
> Date: Mon, 6 Aug 2012 19:42:08 +0200
> Subject: [PATCH] When ldap_group_nesting_level was reached, the LDAP
> provider
>  tried to link group members with groups outside nesting
>  limit.
> 
> https://fedorahosted.org/sssd/ticket/1194
> ---
>  src/providers/ldap/sdap_async_initgroups.c | 35
> +++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/src/providers/ldap/sdap_async_initgroups.c
> b/src/providers/ldap/sdap_async_initgroups.c
> index 8a837bc..1cc278d 100644
> --- a/src/providers/ldap/sdap_async_initgroups.c
> +++ b/src/providers/ldap/sdap_async_initgroups.c
> @@ -1781,7 +1781,13 @@ save_rfc2307bis_group_memberships(struct
> sdap_initgr_rfc2307bis_state *state)
>      TALLOC_CTX *tmp_ctx;
>      struct rfc2307bis_group_memberships_state *membership_state;
>      struct membership_diff *iter;
> +    struct membership_diff *iter_start;
> +    struct membership_diff *iter_tmp;
>      bool in_transaction = false;
> +    int added;

please use num_added, it will immediately convey this is a counter, as
it is it sounds like a boolean but it not.

> +    int i;
> +    int grp_count;
> +    char **add;
>  
>      tmp_ctx = talloc_new(NULL);
>      if (!tmp_ctx) return ENOMEM;
> @@ -1813,15 +1819,42 @@ save_rfc2307bis_group_memberships(struct
> sdap_initgr_rfc2307bis_state *state)
>      }
>      in_transaction = true;
>  
> +    iter_tmp = membership_state->memberships;
> +    iter_start = membership_state->memberships;
> +
>      DLIST_FOR_EACH(iter, membership_state->memberships) {
> +        /* Create a copy of iter->add array but do not include groups
> outside
> +         * nesting limit. This array must be NULL terminated. */
> +        for (grp_count = 0; iter->add[grp_count]; ++grp_count);

In general please use the x++ form not ++x, we use the former everywhere
else.

> +        add = talloc_zero_array(tmp_ctx, char*, grp_count + 1);
> +        if (add == NULL) {
> +            ret = ENOMEM;
> +            goto done;
> +        }

Zeroing the array is just a waste here, whe you are finished assigning
assign all members then just add a NULL for the last one.
memset are relatively cheap with optimizations but still no reason to
abuse of them when not needed.

> +        added = 0;
> +        for (i = 0; i < grp_count; ++i) {
> +            DLIST_FOR_EACH(iter_tmp, iter_start) {
> +                if (!strcmp(iter_tmp->name,iter->add[i])) {
> +                    add[added] = iter->add[i];
> +                    ++added;
> +                    break;
> +                }
> +            }
> +        }
> +        if (add[0] == NULL) {
> +            /* Nothing to add. Skip. */
> +            continue;
> +        }

This becomes:
if (num_added == 0) {
	continue;
} else {
	add[num_added] = NULL;
}


The major question I have is that this new code introduces O(N^2)
behavior, if there are more then a handful of groups it will be quite
costly. Can we find a way that is not so expensive ?

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list