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