On Tue, 2013-02-12 at 16:14 +0100, Martin Kosek wrote:
> Explained in the commit description - this may not be super-critical, I just
> followed info in ldap_search_ext() man page:
> ...
>        Note that res parameter  of  ldap_search_ext_s()  and  ldap_search_s()
> should  be  freed  with
>        ldap_msgfree() regardless of return value of these functions.
> ...


> > This snippet seem to change the logic.
> > 
> > Before the condition was !ipadb_need_retry() and no you change it to
> > ipadb_need_retry() so it looks reversed, was this on purpose ?
> As noted in commit description, this check was wrong and it was causing the
> function to _always_ retry at least once because ipadb_need_retry returns true
> when we need to retry and not 0 -> thus no "!" is needed.

I do not like much 'sneaking' this kind of change in the same patch that
fixes memory leaks, so if you want to plit in 2 patches it would be
nice, but I am of course ok with the change.

> > Why all this copying around ?
> > It should be sufficient to call krb5_free_data_contents() at the end at
> > most.
> You are right, I overengineered this one. Fixed.

Much better now, thanks.

> Attaching a patch fixing this one + an issue found by Sumit (sigh,
> quick-fix-before-send type of error).

Looks good to me now.


Simo Sorce * Red Hat, Inc * New York

Freeipa-devel mailing list

Reply via email to