UNIX semaphores

Question

I wrote an example program about UNIX semaphores, where a process and its child lock/unlock the same semaphore. I would appreciate your feedback about what I could improve in my C style. Generally I feel that the program flow is hard to read because of all those error checks, but I didn't find a better way to write it. It's also breaking the rule of "one vertical screen maximum per function" but I don't see a logical way to split it into functions.

#include <semaphore.h>
#include <stdio.h>
#include <errno.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/mman.h>

int main(void)
{ 
  /* place semaphore in shared memory */
  sem_t *sema = mmap(NULL, sizeof(sema), 
          PROT_READ |PROT_WRITE,MAP_SHARED|MAP_ANONYMOUS, -1, 0);
  if (!sema) {
    perror("Out of memory");
    exit(EXIT_FAILURE);
  }

  /* create, initialize semaphore */
  if (sem_init(sema, 1, 0) < 0) {
    perror("semaphore initilization");
    exit(EXIT_FAILURE);
  }

  int i, nloop=10;
  int ret = fork();
  if (ret < 0) {
    perror("fork failed");
    exit(EXIT_FAILURE);
  }

  if (ret == 0) { 
    /* child process*/
    for (i = 0; i < nloop; i++) {
      printf("child unlocks semaphore: %d\n", i);
      sem_post(sema);
      sleep(1);
    }
    if (munmap(sema, sizeof(sema)) < 0) {
      perror("munmap failed");
      exit(EXIT_FAILURE);
    }
    exit(EXIT_SUCCESS);
  }

  if (ret > 0) {
    /* back to parent process */
    for (i = 0; i < nloop; i++) {
      printf("parent starts waiting: %d\n", i);
      sem_wait(sema);
      printf("parent finished waiting: %d\n", i);
    }
    if (sem_destroy(sema) < 0) {
      perror("sem_destroy failed");
      exit(EXIT_FAILURE);
    }
    if (munmap(sema, sizeof(sema)) < 0) {
      perror("munmap failed");
      exit(EXIT_FAILURE);
    }
    exit(EXIT_SUCCESS);
  }
}

Answer 1

Your code is good, especially the error checks. Error checking always takes up space and can be distracting, but good error checking is a mark of good code (and of a good programmer).

As you say, main is a bit long, but it can be shortened quite nicely by extracting the two for-loops into functions. You can also remove the duplicate munmap call. Here's what you get after the semaphore initialisation:

int pid = fork();
if (pid < 0) {
    perror("fork");
    exit(EXIT_FAILURE);
}
else if (pid == 0) {
    child(sema, nloops);
}
else {
    parent(sema, nloops);
    int stat;
    wait(&stat);
    if (sem_destroy(sema) < 0) {
        perror("sem_destroy");
        exit(EXIT_FAILURE);
    }
}
if (munmap(sema, sizeof *sema) < 0) {
    perror("munmap");
    exit(EXIT_FAILURE);
}
exit(EXIT_SUCCESS);

Note that I renamed ret as pid, as it holds a process ID. And I added a wait call after the parent loop completes so that the parent waits for the child to exit (stat holds the child exit status). I also removed the word "failed" from the perror calls, as the message perror prints will make it clear that the call failed.

Some other points: my Mac's mmap takes MAP_ANON not MAP_ANONYMOUS - which system are you using? And also there is a MAP_HASSEMAPHORE flag that would be appropriate here, although your system may not have it (no idea exactly what it does though).

Finally, the mmap/munmap calls use sizeof(sema) which is the size of the pointer. If it works, I would guess that mmap is actually mapping a whole page and ignoring the size. However, it should be sizeof *sema or sizeof(sema_t)

Answer 2

• main() must always return an int. return 0 at the end of your function.
• The consensus is that, when possible, return instead of exit.
• You could make a helper function that accepts a bool (from stdbool.h) and a const char *, and if the bool is false then perror and exit(EXIT_FAILURE) are called.

Answer 3

One thing to note:

sem_t *sema = mmap(NULL, sizeof(sema), 
         PROT_READ |PROT_WRITE,MAP_SHARED|MAP_ANONYMOUS, -1, 0);
if (!sema) {

Failure in mmap will return MAP_FAILED, not NULL (usually I think MAP_FAILED expands to -1 cast to a pointer, meaning your failure check won't work).

As for style: This is just an opinion, but I think your style could benefit from the concept of a "cleanup block".

For example, instead of this kind of style [paraphrasing your code, not doing literal quotes]:

sema = mmap(/* ... */);

if (sema == MAP_FAILED)
{
   exit(EXIT_FAILURE);
}

foo = malloc(sizeof(*foo));
if (!foo)
{
   exit(EXIT_FAILURE);
}

You could do something like:

   int status = EXIT_FAILURE;
   sem_t *sema = NULL;
   struct foo *foo = NULL;

   sema = mmap(/* ... */);

   if (sema == MAP_FAILED)
   {
      goto cleanup;
   }

   foo = malloc(sizeof(*foo));
   if (!foo)
   {
      goto cleanup;
   }

   // TODO: do stuff with sema and foo

   // mark success
   status = EXIT_SUCCESS;

cleanup:
   if (sema && sema != MAP_FAILED)
      munmap(sema, /* ... */);
   if (foo)
      free(foo);

   return status;

The benefit to this kind of style is that you can add any type of allocation you want (in between the other stuff, before it, after it, whatever), and you just need to add a quick line or two in the cleanup block, and suddenly, all success and failure paths get the resources freed. If any of the intermediate steps fail and you wind up in the cleanup block, you can be assured that you're not leaking anything, and it won't feel repetitive to make that happen.

There are other variants of this, for example if you or someone you're working with has some religious objection to goto (even though it's the cleanest way to do error handling in plain C), on the slightly more repetitive side you could repeatedly check status to see that you're still succeeding:

int status = EXIT_SUCCESS;
sem_t *sema = NULL;
struct foo *foo = NULL;

if (status == EXIT_SUCCESS)
{
   sema = mmap(/* ... */);

   if (sema == MAP_FAILED)
   {
      status = EXIT_FAILURE;
   }
}

if (status == EXIT_SUCCESS)
{
   foo = malloc(sizeof(*foo));
   if (!foo)
   {
      status = EXIT_FAILURE;
   }
}

if (status == EXIT_SUCCESS)
{
   // TODO: do stuff with sema and foo
}

if (sema && sema != MAP_FAILED)
   munmap(sema, /* ... */);
if (foo)
   free(foo);

return status;