On 06/08/2010 07:09 AM, Stephen Gallagher wrote:
On 06/08/2010 03:00 AM, Sumit Bose wrote:
> On Mon, Jun 07, 2010 at 04:02:26PM -0400, Stephen Gallagher wrote:
>> Sumit, I'm requesting that you perform this review, as you are most
>> familiar with this bit of code.
>>
>>
>> Under certain circumstances, the openldap libraries will continue
>> internally trying to reconnect to a connection lost (as during a
>> cable-pull test). We need to drop the reconnection callbacks when
>> marking the backend offline in order to guarantee that they are
>> not called with an invalid sdap_handle.
>>
>> This addresses
https://fedorahosted.org/sssd/ticket/494
>
> Unfortunately I was not able to reproduce #494 here, but if this patch
> works for you I'm fine with it. I only have two minor nitpicks:
>
> - why don't you use talloc_zfree directly?
Because for some reason I haven't yet worked out, talloc_zfree() was
giving me a syntax error. In the interest of having a working patch, I
just bypassed it.
Ok, I realized what was wrong. talloc_zfree() does not return a value,
so it can't be on the right side of an equals. :)
However, I really do want to check the return value here, since one of
the attached destructors can legitimately return -1 here. So I'm leaving
this unchanged.
> - remove_ldap_connection_callbacks() has a return value, so you
should
> either check it and print a debug message in the case of an error or
> define it as void
>
Thanks, I'll fix that
Fixed. It now prints a debug message and adds a comment that even on
failure we're going to attempt to proceed.
.
> This problem might have its origin in 'Better handle sdap_handle memory
> from callers' where we remove a number of talloc_zfree(sdap_ctx->gsh)
> calls.
Yeah, that seems likely
>
> I would prefer to ACK this patch only for the 1.2 branch to solve #494
> for this branch. Because it is planned to improve the handling of the
> global LDAP connetion for 1.3 I hope a cleaner solution can be found for
> master.
>
Would you mind acking it also for master (once this passes review) until
we get to that fix? I don't want to leave a serious known bug in the
tree for an indeterminate amount of time.
--
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.
http://www.redhat.com/promo/vendor/