HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

UNIX semaphores

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
semaphoresunixstackoverflow

Problem

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

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) {
    /* 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);
  }
}

Solution

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)

Code Snippets

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

Context

StackExchange Code Review Q#27000, answer score: 4

Revisions (0)

No revisions yet.