Re: [SSSD] [PATCHES] Support native IPA netgroups

[Stephen Gallagher]

On Wed, 2011-11-16 at 14:11 +0100, Jan Zelený wrote:
> > On Thu, 2011-11-03 at 23:22 +0100, Jan Zeleny wrote:
> > > Stephen Gallagher  wrote:
> > > > On Wed, 2011-10-19 at 00:42 +0200, Jan Zeleny wrote:
> > > > > Jan Zelený  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
>
> I'm sending updated set of patches. All your comments were addressed, I also 
> added man page updates and one modification to sdap_parse_search_base() which 
> can be now used for at more places in IPA provider.
>
> Thanks
> Jan

Ack to all six. Good work!