[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: commit: ldap/libraries/libldap_r rmutex.c threads.c

Hallvard B Furuseth wrote:
Howard Chu writes:
This is just a quickie to get the functionality working.

It doesn't though:-)

Opheader.oh_tid (alias Operation.o_tid) is never set, yet
overlays/accesslog.c uses the value as the owner thread ID.

Did it exist once and get replaced with the 'ctx' things, or
something like that? Anyway, I suppose the variable can be
moved from the Operation structure to accesslog's private data.

Hm, I don't know what happened with oh_tid, but I think it would be better to actually set it, in case others need the thread ID in the future. On some systems pthread_self() is an expensive call, best to avoid calling it needlessly, especially since we have only a small pool of threads; we can get the thread IDs once and never need to ask again.
A few notes on rmutex:

- I don't see why you wrap the struct it in a pointer. It does mean one
can move an rmutex in memory, but it replaces a mutex variable which
couldn't be moved moved.

Just copying the approach from rdwr.c. In particular, this keeps the rmutex itself an opaque structure, which I think is desirable overall, without preventing its use in other structure declarations, etc.
- In ldap_pvt_thread_rmutex_unlock(), since you do error checking here:
ldap_pvt_thread_mutex_lock( &rm->ltrm_mutex );
if( !ldap_pvt_thread_equal( owner, rm->ltrm_owner )) {
ldap_pvt_thread_mutex_unlock( &rm->ltrm_mutex );
I'd also do replace the ldap_pvt_thread_mutex_lock() with
rc = ldap_pvt_thread_mutex_lock( &rm->ltrm_mutex );
if( rc )
return rc;
and then maybe the other test can just as well be deleted, since
slapd doesn't check the return value anyway:-) If so, the 'owner'
argument to ...unlock() can be dropped.

Removing the test makes no sense, ltrm_mutex is not held for the duration of the rmutex.

 -- Howard Chu
 Chief Architect, Symas Corp.  http://www.symas.com
 Director, Highland Sun        http://highlandsun.com/hyc
 OpenLDAP Core Team            http://www.openldap.org/project/