> I will take a look at reworking the
>
> On 08/22/10 10:09 AM, masar...@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.
