patterncMinor
UNIX semaphores
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,
extracting the two for-loops into functions. You can also remove the duplicate
Note that I renamed
child to exit (
"failed" from the
it clear that the call failed.
Some other points: my Mac's
which system are you using? And also there is a
would be appropriate here, although your system may not have it (no idea
exactly what it does though).
Finally, the
pointer. If it works, I would guess that
page and ignoring the size. However, it should be
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 byextracting 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 await call after the parent loop completes so that the parent waits for thechild to exit (
stat holds the child exit status). I also removed the word"failed" from the
perror calls, as the message perror prints will makeit 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 thatwould 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 thepointer. If it works, I would guess that
mmap is actually mapping a wholepage 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.