On Wed, 2011-11-02 at 14:23 +0100, Jan Zelený wrote:
> On Wed, 2011-10-19 at 00:38 +0200, Jan Zeleny wrote:
> > Jan Zelený <jzeleny(a)redhat.com> wrote:
> > > > Steve,
> > > > I finally finished the review:
> > > >
> > > > #1-9: ACK
> > > >
> > > > #11:
> > > > - mention what other values are acceptable for scope (you have only
> > > > subtree in the example)
> > > > - maybe mention a reference to a document about LDAP filters? Or a
> > > > brief description how the filter can look like. This is not
> > > > necessary, just a thought to avoid some user questions in the
> > > > future.
>
> More detail added to the manpage
>
> > > > Consider following patches ACKed, I leave suggested changes at your
> > > > discretion:
> > > >
> > > > #10:
> > > > sdap_initgr_rfc2307bis_send
> > > > - how about making one fail/done label and some gotos?
>
> Agreed and done.
>
> > > > sdap_initgr_rfc2307bis_process
> > > > - I'd rather use memcpy than the cycle
>
> I disagree. I find memcpy to be hard to read. With the cycle, it's clear
> exactly what assignment is being made. (Besides, gcc will probably
> optimize to the same result).
>
> > > > #13:
> > > > sdap_get_users_process
> > > > - I'd rather use memcpy than the cycle
>
> See above
>
> > > > #14:
> > > > sdap_get_groups_process
> > > > - I'd rather use memcpy than the cycle
>
> See above
>
> > > Implementing multiple search bases into IPA provider, I noticed today
> > > something else in patch #0006.
> > >
> > > in function sdap_get_netgroups_process() you call talloc_zfree() on two
> > > variables. I believe both of those are redundant:
> > > - if count is zero then state->netgroups will be NULL
> > > - state->filter is either freed in repeated call of
> > > sdap_get_netgroups_next_base() or it will be freed with the entire
> > > context due to an error.
> > >
> > > I noticed similar code also being in patches #0005, #0007, #0008,
> > > however I didn't inspect it very deeply there, so I don't know
for
> > > sure that it is redundant there as well.
>
> You're right, they were redundant. Harmless, but better to be defensive
> so we don't have to fix two places in the future if we change something.
> Fixed.
>
> > One more nack,
> > I noticed that search bases aren't parsed in IPA provider when they are
> > explicitly set in config file.
>
> Fixed.
>
> New patches attached are rebased atop the current master.
Ack to all
Pushed to master.