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.