[PATCH 3/3] Abstract unnamed semaphore

[Boris Dušek]

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. :-)

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.

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.

>> 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".

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
to add additional checks you suggest below like exit on sem_* function
failure etc.

>> +/**
>> + * spd_sem_t and functions below behave like a POSIX unnamed semaphore
>> + */
>> +struct spd_sem_s;
>> +typedef struct spd_sem_s spd_sem_t;
> 
> nit, I think just typedef struct x    x_t; is equally valid.

if you say so, I don't know :-)

>> +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

> I'm not actually sure if that would link,

structs and typedefs don't affect linking in C - they are not symbols

> but I believe it should.

it does :-) (and in case you meant compiling instead of linking, it does too)

> 
>> +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.

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.

>> +{
>> +    spd_sem_t *sem;
>> +    int err;
> 
> what do you need the variable for?

right, I don't need it

>> +    sem = (spd_sem_t *) malloc(sizeof(spd_sem_t));
> 
> you could do that where you declare it no?

yes, especially now that the other variable declaration (int err) is gone

>> +    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."

>> +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

>> 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

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 ...

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.