On Thu, 2011-11-03 at 23:22 +0100, Jan Zeleny wrote:
Stephen Gallagher sgallagh@redhat.com wrote:
On Wed, 2011-10-19 at 00:42 +0200, Jan Zeleny wrote:
Jan Zelený jzeleny@redhat.com wrote:
I'm sending couple patches which add support for IPA netgroups:
#53 These routines were not static, so I renamed them in order to avoid confusion and possible collision with equivalent routines in IPA provider
Ack.
#54 Some new config options, please focus on this patch, I'm not entirely sure if my approach was the correct one.
Nack.
Looks mostly good, except that when you added the new host search base option, you don't check it in the ipa_get_id_options() function. You need to make sure to set it appropriately and call sdap_parse_search_base().
#55 This new context was necessary so I can pass ipa options to routine determining host search base.
Ack.
#56 This is netgroups support itself
Nack.
As a nitpick, I'd prefer if you used the label 'done' in ipa_get_netgroups_process() instead of 'fail', since it's possible to reach there in the ENOENT case (which is technically a success case).
Did you test this with any netgroups that have more than one member triple? Your use of sysdb_attrs_get_string() will return ERANGE if there is more than one of the requested attribute. You probably want to use sysdb_attrs_get_el() to verify whether there are any values there.
Nitpick: when you 'continue' if entities_found == 0, don't use the else block. Save yourself an indentation level.
Please don't use tmp_str for anything useful (like the sss_filter_sanitize() or the hash key). It's confusing to figure out which one you're sanitizing.
ipa_netgr_fetch_*() functions do not handle multiple search bases (or the associated filters).
get_host_search_base() should not be occurring in ipa_netgr_fetch_hosts(), it should be set up like users and netgroups in the ipa_get_id_options() function and capable of handling multiple search bases.
Would you please add more comments to ipa_netgr_members_process()? It's nicely generic, but that makes it tricky to follow.
Otherwise, I like the approach. Good work!
#57 IPA id provider which is utilizing previously added support of netgroup fetching
Ack