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

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



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.

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.

- 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 );
		return LDAP_PVT_THREAD_EINVAL;
	}
  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.

-- 
Hallvard