Hi,
On Thursday 16 September 2010 20:16:56 Simo Sorce wrote:
On Thu, 16 Sep 2010 17:50:28 +0200
Ralf Haferkamp <rhafer(a)suse.de> wrote:
> Hi,
>
> On Thursday 09 September 2010 15:14:10 Ralf Haferkamp wrote:
[..]
> Find a newer version of my patch attached. Actually it's 3
patches
> now. Please review.
>
> Patch1: This just adds a new flag to save_groups() to indicate that
> the group's member attribute is already populated with the members'
> sysdb DN (instead on LDAP DNs). As I need to lookup the group
> members in sysdb anyway, when processing the group, this saves some
> redundant
>
> sysdb lookups when storing the group.
This looks like a good idea.
> Patch2: This is a somewhat improved version of my last patch.
>
> - better error handling
> - limit the number of LDAP requests that are issued before
[..]
This patch makes the main function very complex, I suggest that you
at
least create separate functions for each new tevent request you want
to create, that is sort of a rule for sssd. (And it makes code
digestible more often than not).
Find a new version attached. Does that look
better? If that is not what
you were referring to lets discuss it in IRC on monday.
Note, I needed to implement sdap_process_group_send() slightly differnent
than the other _send functions in sdap_async_accounts(). Instead of
returning as tevent_req* it does return a error code. The tevent_req* is
returned via the argument list. I did this because there are cases where
sdap_process_group_send() does not need to create a new tevent_req*, e.g.
when the groups doesn't have any members or when all groupmembers are
already cached.
As for group unrolling I have also started working on it (ticket
#625), although I am doing that in the 1.2.x branch as we need the
functionality there too. I will try to post a patch soon so that we
can compare relative approaches and merge the effort, ok ?
Fine. Feel free to take
what you need from my patch.
> Patch3: This adds a new config option to
"ldap_unroll_group_members"
> to enable/disable group unrolling
Can we use the followin patch instead ?
http://fedorapeople.org/gitweb?p=simo/public_git/sssd.git;a=commitdiff
;h=fedf324be284de71e5dbf22f0135e9f681a15bde
This patch assumes the code will consider a nesting level of 0 as "no
nesting". therefore it will embed in a single option both a way to
enable disable unrolling and a limit on the level of nesting we will
allow on the client (to avoid loops or very long delay on pathological
cases).
Hm, just to make sure I understand your approach:
"ldap_group_nesting_level = 0" would be equal to
"ldap_unroll_group_members = false" -> resulting in the current getgrnam
behavior of only returning cached members.
"ldap_group_nesting_level = 1" would mean "ldap_unroll_group_members =
true" -> return only direct members of the group and ignore mested
groups.
while "ldap_group_nesting_level > 1" allow nesting up to a certain depth.
If that is correct what does "ldap_group_nesting_level = 0" mean for the
initgroups() call? Return only groups that are already cached? I wonder
if that would be a good idea.
--
Ralf