On Wed, 2011-05-18 at 16:41 +0200, Jakub Hrozek wrote:
> 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.
Patch 0001: Ack
Patch 0002: Ack
Patch 0003: Ack
Patch 0004: Ack
Patch 0005: Ack
Patch 0006: Ack
Patch 0007: Ack
Patch 0008: Ack
Patch 0009: Nack. Please add ldap_deref_threshold to sssd-ipa.conf (not
just sssd-ldap.conf.
Done. I also noticed that ldap_page size was missing, so I added that
one, too.