[PATCH 3/3] Abstract unnamed semaphore

[Trevor Saunders]

On Tue, Jul 24, 2012 at 10:08:33PM +0200, Boris Du?ek wrote:
> From: Boris Dus?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?

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

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

> +struct spd_sem_s {
> +     sem_t sem;
> +};

why not just use a typedef?  I'm not actually sure if that would link,
but I believe it should.

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

> +{
> +     spd_sem_t *sem;
> +     int err;

what do you need the variable for?

> +     sem = (spd_sem_t *) malloc(sizeof(spd_sem_t));

you could do that where you declare it no?

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

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

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

Trev