On 05/09/2011 07:23 PM, Stephen Gallagher wrote:
Continuing my review. Comments inline.
New patches are attached. They are formatted with -M -C --patience
--full-index, per Simo's suggestion. I think it helped, especially with
patch #1.
I'd like to merge the replies into one thread. Below are replies for the
first half of patches.
Patch #1 has no changes.
Patch #2 is a new one that just defines a couple of data structures and
one function that are used in later patches in order to process MPGs.
Patches #3 and #4 (previously #2 and #3) that implement the
platform-specific dereference methods are now able to dereference MPGs,
too. The OpenLDAP patch also removes one unused variable.
If an LDAP object that matches multiple maps is dereferenced, it is
returned as multiple sysdb_attrs * results plus the corresponding map to
speed up processing.
I tested the functionality by dereferencing an object that was both a
posixUser and a posixGroup. The dereference itself worked fine and both
objects were saved to sysdb but it seems that the rest of the LDAP
provider assumes that originalDN is unique -- so later the back end
failed when it was trying to build memberships. This is something to fix
if we want to support MPGs, but I don't think it is related to this
patchset.
Patch #5 (previously #4) now has single sdap_deref_search_done()
function instead of one per method.
The patch that changed append_attrs_to_array was retired. I will send a
separate patch to remove that function altogether as it is not used
anywhere.
Patch #6 has no changes.
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.
Fixed.
> [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.
Fixed
> [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.
Fixed
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.
Making it configurable is a good answer :-) and I agree that the option
should only specify absolute values.