[PATCH 3/3] Abstract unnamed semaphore

[Trevor Saunders]

On Wed, Jul 25, 2012 at 01:08:29PM +0200, Boris Du?ek wrote:
> 25. 7. 2012 v 6:25, Trevor Saunders:
> 
> > On Tue, Jul 24, 2012 at 10:08:33PM +0200, Boris Du?ek wrote:
> >> From: Boris Du?ek <dusek at brailcom.org>
> >> 
> >> Some systems lack unnamed semaphores. So wrap our use of unnamed
> > 
> > really?  I guess there is a non posix.1-2001 OS out there you think it
> > makes sense to care about?
> 
> Yep. OS X. UNIX 03 certified. :-)

ahh, ugh :(

> I remember looking into this sometime ago and discovered that sem_ functions
> are part of some optional POSIX module that is not in the base POSIX 
> specification.
> That might have changed in recent versions but probably not in the
> version Apple was targeting.

interesting, I didn't look farther than the linux man page which claims
simply posix.1-2001 which afaik was the first version.

> And of course not that if theoretically SD is to ever run on Windows,
> then such thing would be needed as well - I guess Windows does have
> unnamed semaphores, but definitely with a different API.

Well, on windows without cygwin or msys, but that seems like a silly
efort and I'm not sure why someone would bother when windows comes with
a speech api.  And if for some someone came up with a good reason to
support windows natively I'd think we'd really want to consider witching
to c++.

> >> semaphores in thin wrappers around sem_* function to be able to add
> >> different implementations of these wrappers on systems that need it.
> > 
> > I'd suggest instead of doing this adding a configure check, and then on
> > mutant systems that don't have semaphores we can have our own impl
> > providing the posix interface.
> > 
> > That way new comers don't have to go digging to figure out the details
> > of how our own semaphore api works.
> 
> Well the interesting point is that sem_init (and the rest of sem_* functions)
> *is* present on OS X, it "just" returns error and errno that specify "Not 
> implemented".

ugh!

> So not doing anything on Linux and defining our own sem_init on OS X
> would require some "hiding of symbols" stuff (by linker or preprocessor
> definitions) which would seem clumsy to me. It would also not allow

true, you could probably just have config.h include #define sem_x
spd_sem_x but still ugly I agree.

Given this your approach seems reasonable to me :/

> >> +struct spd_sem_s {
> >> +  sem_t sem;
> >> +};
> > 
> > why not just use a typedef?
> 
> the typedef is already handled in the header file, here we only need
> to define the (contents of the) struct itself

oh, yeah must have been tired when I wrote that the type you produce
must be struct spd_sem_s not just spd_sem_s.

> > I'm not actually sure if that would link,
> 
> structs and typedefs don't affect linking in C - they are not symbols

fair point, I was thinking C++ where structs do matter if theyy are
return types or arg types.

> >> +spd_sem_t*
> >> +spd_sem_create(unsigned count)
> > 
> > I'd say you should provide a form that takes the semaphore to init as an
> > arg so when possible malloc()ing can be reduced.  Note its also just
> > faster to take the address of a global than to pass a global to a
> > function.
> 
> I thought about that but it is unusable and I actually have a reason
> why it is this way. Here is why:
> 
> How struct spd_sem_s looks is dependent on the platform. We don't
> want to put how it looks (i.e. the definition of the struct) in the
> header since it is an implementation detail (think public vs. private
> in OOP). So common for this is to forward-declare the struct (and optionally
> a convenient typedef) in header file, but define the contents of the struct
> in implementation file. The API then uses only pointers to the struct, since
> sizeof(any pointer, including that struct) is compile-time constant even
> for incomplete types as our struct.
> 
> Making struct spd_sem_s usable as a non-pointer variable would require
> the whole definition of the struct to be known to all clients, i.e.
> definition of spd_sem_s struct in the header file. But we don't want
> to do that because what struct spd_sem_s is our private implementation
> business and of no interest to the clients.

I understand your desire to actually hide things but you could make the
same argument for sem_t itself or pthread_*_t.

Basically I'd agree in principal the client shouldn't know, but its
better in practice for them to technically know, but not do anything
with that knowledge the way they do for pthread_*_t for example.  Note
that any body who tries to have to poke at the internals wil be pretty
obviously doing so since to build they'll need a bunch of ifdefs.  So
basically I think C is C and you should just hide stuff by convention.

I suppose you could declare a dummy struct spd_sem_t that just containsa
char[] for clients, but honestly that seems hard and silly when you
could just put the struct in the header.

> Also the memory and time costs of malloc'ing a spd_sem_t are IMHO negligible
> against the memory/time costs of creating and using such semaphores, since
> they are kernel objects and using them requires transitioning from userspace
> to kernel and back.

I'm not sure how any particular implementation of pthreads does
semaphores, but its not neccessarily true that you will always have to
go through the kernel.  Consider the basic lock / condvar semaphore
where locks are implemented using futex(2).  In the contended case
taking the lock is just an xadd, and changing the count and unlocking
are just add s.

> >> +  if (sem == NULL) {
> >> +          DBG("Could not allocate memory for semaphore");
> >> +          errno = ENOMEM;
> >> +  } else if (-1 == (err = sem_init(&sem->sem, 0, count))) {
> > 
> > personally I'd be more comfortable with < 0 than == -1
> 
> I am following the spec that says "Otherwise, it returns -1 and sets errno to 
> indicate the error."

I know, but I wonder what returning -2 means then if not an error.  It a
being careful thing more than a following the spec thing.

> >> +int
> >> +spd_sem_destroy(spd_sem_t *sem)
> >> +{
> >> +  int ret = sem_destroy(&sem->sem);
> >> +  free(sem);
> >> +  return ret;
> >> +}
> > 
> > I think this function should return void since 1) there is absolutely
> > nothing the caller can other than print an error which you could do here
> > just as well, and 2 it seems from the man page that given a valid
> > semaphore it will never (it might if someone is blocked but that's not
> > defined).
> > 
> > Especially considering we very rarely if ever check they're return
> > values I'd suggest if we're going to write our own semaphore api post()
> > and wait() should also return void since callers can't really handle
> > those errors either, and so failure should probably just be fatal.
> > Fatal errors seem especially nice in modules where the server can
> > easily restart them.
> 
> that's a good idea, I will do, seems reasonable to increase the robustness
> of using semaphores when we have such an abstraction and can thus do it all
> in one place

yeah, if we have to live with non standard api's we might as well make
them good :)

> 
> >> diff --git a/src/modules/module_utils.h b/src/modules/module_utils.h
> >> index 031692f..10082ad 100644
> >> --- a/src/modules/module_utils.h
> >> +++ b/src/modules/module_utils.h
> >> @@ -1,7 +1,7 @@
> >> +#include <spd_utils.h>
> > 
> > speechd is a small enough project its not a big deal, but whhy not use a
> > forward decl since this is a header?
> 
> Yes, but I got errors with type redefinitions since then spd_sem_t is declared
> in both the spd_utils.h header and in the forward declaration in 
> module_utils.h

 I wonder why, multiple struct foo;s are valid I'm pretty sure.

> Real solution would be to have the declaration of spd_sem_(s,t) in even 
> another
> header (e.g. spd_utils_fwd.h) and then include it from both module_utils.h and
> spd_utils.h. But that extra header would seem to me to create more noise ...

I suspect our header situation could just generally use a bit of work :/

Trev

> Thanks for your comments so far. Please let me know what you think about
> the above so that I can work on improved version of the patch and resend
> it for review.
> _______________________________________________
> Speechd mailing list
> Speechd at lists.freebsoft.org
> http://lists.freebsoft.org/mailman/listinfo/speechd