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

Stephen Gallagher sgallagh at redhat.com
Tue Jun 8 12:03:45 UTC 2010


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/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Disable-connection-callbacks-when-going-online.patch
Type: text/x-patch
Size: 3220 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20100608/5f0c9e54/attachment.bin>


More information about the sssd-devel mailing list