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