[Freeipa-devel] [PATCH 0020] Separate LDAP result from LDAP connection, fix deadlock.

[Petr Spacek]

On 05/15/2012 02:32 PM, Adam Tkac wrote:
> On Mon, May 14, 2012 at 04:44:42PM +0200, Petr Spacek wrote:
>> On 05/11/2012 12:26 PM, Adam Tkac wrote:
>>> On Mon, May 07, 2012 at 02:49:07PM +0200, Petr Spacek wrote:
>>>> Hello,
>>>>
>>>> this patch fixes https://fedorahosted.org/bind-dyndb-ldap/ticket/66:
>>>> Plugin deadlocks during new zone load when connections == 1.
>>>>
>>>> It fixes structural problem, when LDAP query result was tied with
>>>> LDAP connection up. It wasn't possible to release connection and
>>>> work with query result after that.
>>>> Described deadlock is consequence of this problematic design.
>>>>
>>>> Now LDAP connection is separated from LDAP result. Next planed patch
>>>> will avoid "manual" connection management, so possibility of
>>>> deadlock should be next to zero.
>>>>
>>>> Petr^2 Spacek
>>>
>>> Hello Peter,
>>>
>>> good work, please check my comments below.
>>>
>>> Regards, Adam
>>>
>>>>  From 8ee1fd607531ef71369e99c9228456baea45b65d Mon Sep 17 00:00:00 2001
>>>> From: Petr Spacek<pspacek at redhat.com>
>>>> Date: Mon, 7 May 2012 12:51:09 +0200
>>>> Subject: [PATCH] Separate LDAP result from LDAP connection, fix deadlock.
>>>>   https://fedorahosted.org/bind-dyndb-ldap/ticket/66
>>>>   Signed-off-by: Petr Spacek<pspacek at redhat.com>
>>
>> Hello Adam,
>>
>> thanks for ideas/improvements!
>>
>> Reworked patch is attached. I did all proposed changes except this one:
>>
>> @ ldap_psearch_watcher:
>>>>   restart:
>> (... snip ...)
>>>>   soft_err:
>>>> -
>>>> -			ldap_msgfree(conn->result);
>>>> -			ldap_entrylist_destroy(conn->mctx,
>>>> -					&conn->ldap_entries);
>>>> +			;
>>>
>>> Empty label "soft_err:" is useless, please remove it and use "continue;" on
>>> appropriate places;
>>
>> I think "continue" in this place can lead to memory leak, so I
>> removed soft_err by other way.
>
> Thanks for the patch, now it looks fine to me, except that it doesn't apply on
> the current master:
>
> [atkac at drtic bind-dyndb-ldap]$ git am ../bind-dyndb-ldap-pspacek-0020-2-Separate-LDAP-result-from-LDAP-connection-fix-deadlo.patch
> Applying: Separate LDAP result from LDAP connection, fix deadlock. https://fedorahosted.org/bind-dyndb-ldap/ticket/66 Signed-off-by: Petr Spacek <pspacek at redhat.com>
> error: patch failed: src/ldap_helper.c:271
> error: src/ldap_helper.c: patch does not apply
> Patch failed at 0001 Separate LDAP result from LDAP connection, fix deadlock. https://fedorahosted.org/bind-dyndb-ldap/ticket/66 Signed-off-by: Petr Spacek <pspacek at redhat.com>
> When you have resolved this problem run "git am --resolved".
> If you would prefer to skip this patch, instead run "git am --skip".
> To restore the original branch and stop patching run "git am --abort".
>
> Please rebase the patch and then push it, you don't have to resend it here.
>
> Regards, Adam

Finally, I rebased the patch and pushed it to the master. Sorry for delay, I 
forgot to this ticket completely.

Rebased version is attached.

https://fedorahosted.org/bind-dyndb-ldap/changeset/88dcade344af6e71503b85c4d2630343dbf7d7c0

Petr^2 Spacek
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bind-dyndb-ldap-pspacek-0020-3-Separate-LDAP-result-from-LDAP-connection-fix-deadlo.patch
Type: text/x-patch
Size: 20351 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120712/b96410d2/attachment.bin>