On Tue, Jun 08, 2010 at 08:03:45AM -0400, Stephen Gallagher wrote:
> 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.
ok
>
>>> - 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.
>>
ok, fair enough.
ACK for 1.2 and master
Pushed to master and sssd-1-2.
--
Stephen Gallagher
RHCE 804006346421761
Delivering value year after year.
Red Hat ranks #1 in value among software vendors.