Issue 7363 - libmdb should use POSIX semaphores on non-apple BSD systems too.
Summary: libmdb should use POSIX semaphores on non-apple BSD systems too.
Status: VERIFIED FIXED
Alias: None
Product: OpenLDAP
Classification: Unclassified
Component: build (show other issues)
Version: 2.4.32
Hardware: All All
: --- normal
Target Milestone: ---
Assignee: OpenLDAP project
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-22 20:02 UTC by cmikk@qwest.net
Modified: 2014-08-01 21:04 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this issue.
Description cmikk@qwest.net 2012-08-22 20:02:27 UTC
Full_Name: Chris Miikkelson
Version: 2.4.32
OS: FreeBSD
URL: 
Submission from: (NULL) (204.147.85.37)


mdb on BSD-derived systems other than OS X also needs to use POSIX semaphores
for inter-process synchronization. I was working around this by adding
"-D__APPLE__" to CFLAGS, but that could have side-effects on some systems. The
following patch tells mdb to use posix semaphores if either BSD or __APPLE__ is
defined, which seems like a more targeted fix.

http://mikk.net/~chris/patches/0001-Use-posix-semaphores-on-apple-and-bsd-systems.patch
Comment 1 Howard Chu 2012-08-22 21:10:30 UTC
cmikk@qwest.net wrote:
> Full_Name: Chris Miikkelson
> Version: 2.4.32
> OS: FreeBSD
> URL: 
> Submission from: (NULL) (204.147.85.37)
> 
> 
> mdb on BSD-derived systems other than OS X also needs to use POSIX semaphores
> for inter-process synchronization. I was working around this by adding
> "-D__APPLE__" to CFLAGS, but that could have side-effects on some systems. The
> following patch tells mdb to use posix semaphores if either BSD or __APPLE__ is
> defined, which seems like a more targeted fix.
> 
> http://mikk.net/~chris/patches/0001-Use-posix-semaphores-on-apple-and-bsd-systems.patch
> 
> 
Thanks for the patch, applied.

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

Comment 2 Howard Chu 2012-08-22 21:22:02 UTC
changed notes
changed state Open to Test
moved from Incoming to Build
Comment 3 Quanah Gibson-Mount 2012-08-22 21:27:31 UTC
changed notes
changed state Test to Release
Comment 4 Hallvard Furuseth 2012-08-29 06:07:31 UTC
Please try this patch, I can't.  It's split up for readability.
Replace defined(BSD) in 1st patch with defined(__BSD__) or whatever,
see below.

  http://folk.uio.no/hbf/its7363-cleanup.txt

Also, maybe we can drop __APPLE__ - it may #define __BSD__ too.
ANDROID should be something like __ANDROID__.  I can't test either.


Explanation:

defined(BSD) breaks in strict ISO C mode.  Is there a symbol like
__BSD__ or __BSD to use instead?

Namespace-wise, "MDB_USE_POSIX_SEM" would be better.

MDB_FDATASYNC should not depend on USE_POSIX_SEM.

We can #ifndef PTHREAD_PROCESS_SHARED instead of BSD, unless someone
#define it without supporting it.  Not for MDB_FDATASYNC though.

-- 
Hallvard

Comment 5 Howard Chu 2012-08-29 06:20:51 UTC
h.b.furuseth@usit.uio.no wrote:
> We can #ifndef PTHREAD_PROCESS_SHARED instead of BSD, unless someone
> #define it without supporting it.  Not for MDB_FDATASYNC though.
> 
PTHREAD_PROCESS_SHARED is defined just about everywhere, that's not a useful
test. (You only discover at runtime that the mutex creation/init failed...)

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

Comment 6 Hallvard Furuseth 2012-08-29 06:50:17 UTC
Howard Chu writes:
> PTHREAD_PROCESS_SHARED is defined just about everywhere, that's not a useful
> test. (You only discover at runtime that the mutex creation/init failed...)

Argh.  Oh well, we should only do the namespace cleanup, then.  __BSD__
symbols etc.

BTW, I think the lock file should contain a field indicating the sync
primitive type, so different compilations of MDB won't use different
sync primitives.  Which maybe also means we might as well pick the
primitive at runtime (try PTHREAD_PROCESS_SHARED first, then sem_open).

-- 
Hallvard

Comment 7 cmikk@qwest.net 2012-08-29 14:27:24 UTC
On Wed, Aug 29, 2012 at 08:07:31AM +0200, Hallvard Breien Furuseth wrote:
> We can #ifndef PTHREAD_PROCESS_SHARED instead of BSD, unless someone
> #define it without supporting it.  Not for MDB_FDATASYNC though.

The *BSD systems define PTHREAD_PROCESS_SHARED but do
not implement sharable pthread mutexes. The flag name
is part of the API, but implementation of the behavior
it requests is optional.

-- 
Chris Mikkelson  | Quidquid latine dictum sit, altum viditur
cmikk@qwest.net  | 

Comment 8 Hallvard Furuseth 2012-09-01 11:16:01 UTC
Chris Mikkelson writes:
> The *BSD systems define PTHREAD_PROCESS_SHARED but do
> not implement sharable pthread mutexes. The flag name
> is part of the API, but implementation of the behavior
> it requests is optional.

Yes, sorry about that.

How about something like a "__BSD" preprocessor symbol to replace
"BSD"?  There surely is one.  It'll start with an underscore
followed by either another underscore or an uppercase letter.

-- 
Hallvard

Comment 9 Hallvard Furuseth 2012-09-17 14:22:19 UTC
changed notes
changed state Release to Test
Comment 10 Hallvard Furuseth 2012-09-17 14:22:31 UTC
changed notes
Comment 11 Hallvard Furuseth 2012-09-17 14:28:32 UTC
I've cleaned up preprocessor names, and remove semaphores more
thoroughly - at init, and after ITS#7377 at at exit failure.

Anything else - like SysV semaphores - can wait until
someone shows up who cares about it.

-- 
Hallvard

Comment 12 Hallvard Furuseth 2012-09-17 14:32:07 UTC
changed notes
Comment 13 Hallvard Furuseth 2012-09-17 15:04:38 UTC
Oops, part of last message was for ITS#7364.
Comment 14 Quanah Gibson-Mount 2013-02-26 20:16:27 UTC
changed notes
changed state Test to Release
Comment 15 Quanah Gibson-Mount 2013-03-05 02:17:26 UTC
changed notes
changed state Release to Closed
Comment 16 OpenLDAP project 2014-08-01 21:04:07 UTC
fixed in mdb.master
fixed in RE24, master