[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