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.