Hi Simo,
On 04/13/2010 08:51 PM, Simo Sorce wrote:
This patch attempts to address the problem Eugene found with
sdap_handle_release()
I have checked the suggested patch and found several problems:
1. providers/ipa/ipa_access.c still lacks failover retries
2. Why number of retries is 2? It is only enough for one server. If we
have failover set up we need more retries (at least 1 per server + 1 for
stale cached connection)
3. My patch contained an additional fix to sdap_handle_release. Before
calling op callbacks LDAP callback data was destroyed:
commit: sssd-1_1_1
line 102: talloc_zfree(sh->conncb->lc_arg);
and later LDAP handle was unbound:
line 116: ldap_unbind_ext(sh->ldap, NULL, NULL);
But during unbind the LDAP callbacks are called. And sssd callback
(sdap_ldap_connect_callback_del) accesses zero pointer (lc_arg).
So before freeing sh->conncb->lc_arg it is necessary to call:
/* remove callback first */
ldap_get_option(sh->ldap, LDAP_OPT_CONNECT_CB, sh->conncb);
Actually I wonder how does it work for you without this fix? Probably
you have never tried to close a connection with still open file descriptor.
4. The suggested patch is a workaround. It relies on the agreement that
all _send functions will be called from top level event context, not
from subreq callback context. This is hard to guarantee.
I have attached to this e-mail a version of my patch addressing the same
problem that applies directly on top of sssd 1.1.1.
I think its better to use my solution as:
1. It provides a real fix for the problem rather than workaround
2. The complexity of patches is alike (5 files, 213 versus 138 changed
lines)
3. The real changes in my patch are localized to sdap.h,
sdap_async_private.h, sdap_handle_create, sdap_handle_release
4. Other changes are cosmetic and related to changes in structure
definition and therefore can be easily checked by peer review
Regards, Eugene