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 <sgallagh(a)redhat.com> wrote:
> > > On Wed, 2011-10-19 at 00:42 +0200, Jan Zeleny wrote:
> > > > Jan Zelený <jzeleny(a)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
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!