Re: Proposed patch to support: draft-zeilenga-ldap-c-api-concurrency

[masarati@aero.polimi.it]

>   I will take a look at reworking the
>
> On 08/22/10 10:09 AM, masarati@aero.polimi.it wrote:
>>> My concern is: can you guarantee that the occurrences of
>>> locking/unlocking
>>> those additional mutexes, combined to the existing ones, do not result
>>> in
>>> deadlocks?  I mean: did you explicitly check all possible logical paths
>>> or
>>> so, or take measures to avoid this possibility?
>> Another mostly "style" comment: I think you shouldn't be polluting
>> libldap's space with thread implementation specific details, like
>>
>> init.c:
>> #if defined(HAVE_THR)
>> #elif defined(HAVE_PTHREADS)
>>      PTHREAD_MUTEX_INITIALIZER
>> #if !(defined(HAVE_THR) || defined(HAVE_PTHREADS))
>>
>> and so.  I understand PTHREAD_MUTEX_INITIALIZER is handy, but the whole
>> thing should remain confined in libldap_r specific files.
>
> I will take a look at reworking this for the next round of code review.

You could

#if defined(HAVE_THR)
#define LDAP_PVT_MUTEX_INITIALIZER DEFAULTMUTEX
#elif defined(HAVE_PTHREADS)
#define LDAP_PVT_MUTEX_INITIALIZER PTHREAD_MUTEX_INITIALIZER
#else
#define LDAP_PVT_MUTEX_INITIALIZER NULL
#endif

then use LDAP_PVT_MUTEX_INITIALIZER in code; you could then add a
ldap_pvt_thread_mutex_init_if(mutex) that assumes mutex was initialized
with LDAP_PVT_MUTEX_INITIALIZER; for THR and PTHREADS this call should be
empty, while in the remaining cases it would call
ldap_pvt_thread_mutex_init() only if mutex is not equal to
DAP_PVT_MUTEX_INITIALIZER (NULL).

Also, I get some nasty warnings from other parts of the code that
(admittedly incorrectly) include ldap-int.h because
LDAP_PVT_THREAD_ASSERT_MUTEX_OWNER() is being redefined.  I suggest that
instead of #defining it if LDAP_R_COMPILE is not defined, you rather

#ifdef LDAP_R_COMPILE
#define LDAP_ASSERT_MUTEX_OWNER(mutex) \
        LDAP_PVT_THREAD_ASSERT_MUTEX_OWNER( mutex )
#else
#define LDAP_ASSERT_MUTEX_OWNER(mutex)
#endif

so you avoid mixing LDAP_PVT_THREAD_* and LDAP_* macros.

Thanks, p.