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(a)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