Continuing my review. Comments inline.
On Wed, 2011-05-04 at 13:19 +0200, Jakub Hrozek wrote:
[PATCH 7/9] Use fake users during RFC2307bis nested group processing
Instead of downloading complete user data which is potentionally very
slow, only download the necessary minimum information and store the
users as dummy entries.
Nack.
sdap_nested_group_populate_users() doesn't properly sanitize the origDN
before using it in a search filter.
[PATCH 8/9] Refactor RFC2307bis nested group processing
This patch splits checking cache and hash tables into standalone
functions. This will make it easy to reuse the code in a new branch that
uses dereferencing.
Nack.
I realize this was a problem in my original code, but would you mind
fixing two issues with sdap_nested_group_check_cache() while you're
there?
1) Please rearrange the code so that all of the user processing takes
place before the group processing. It's not terribly readable the way
the complete group processing happens in the middle. It would be better
to have the return value from sysdb_search_users() set mtype either to
SYSDB_MEMBER_USER or SYSDB_MEMBER_GROUP (assumed) and then have another
IF statement after that so we can do all of the user processing in
series.
2) It's kind of wasteful to allocate filter twice in the case of groups.
Better would be to hang onto it until we're sure we're not processing a
group. Then we don't need to allocate and free the exact same filter
twice.
Please change the comment in the ENOENT block of
sdap_nested_group_process_step() since it no longer makes sense with the
user and group split in a sub-function.
[PATCH 9/9] Use dereference in processing RFC2307bis nested groups
Instead of issuing N LDAP requests when processing a group with N users,
utilize the dereference functionality to pull down all the members in a
single LDAP request.
https://fedorahosted.org/sssd/ticket/799
Nack.
In sdap_nested_group_process_deref_step(), if
sdap_nested_group_check_cache() returns something other than EOK, ENOENT
or EAGAIN, it should be returned as an error. Right now, you're treating
all errors as ENOENT.
Regarding the tuning for when to perform the deref, I don't have a
perfect answer either. My only suggestion would be to make the value
configurable in sssd.conf. I think we should get rid of the percentage
and make it purely based on number of members. The way to tune it should
be that we should deref whenever a deref will take less time to process
than N members. I think we should default to ten members.