[SSSD] [PATCH] Disable connection callbacks when going online

Stephen Gallagher sgallagh at redhat.com
Wed Jun 9 12:36:11 UTC 2010


On 06/08/2010 08:20 AM, Sumit Bose wrote:
> 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.
http://www.redhat.com/promo/vendor/



More information about the sssd-devel mailing list