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

A simple fork with an ugly waitpid

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

Problem

This is my first take on fork, exec and waitpid. I didn't quite understand why I had to use a while loop with waitpid. I discovered that waitpid sets errno to EINTR which is translated to this:

#define EINTR        4  /* Interrupted system call */


And I saw a suggestion in comp.unix.programmer archives to use a while loop for waitpid.

My implementation works on OSX 10.10.3 but it doesn't quite satisfy me. Can you provide suggestions?

main.c

#include 
#include 
#include 
#include 
#include "common.h"

int main(int argc, const char * argv[]) {

    int exit_status_raw;

    pid_t pid = fork();

    if (pid  0) {
        printf("PARENT says Hello\n");

        while ((pid = waitpid(pid, &exit_status_raw, 0)) == -1) {
            switch (errno) {
                case EINTR:
                    printerr("waitpid failed with EINTR. Retrying...");
                    continue;
                default:
                    printerr("waitpid failed");
                    break;
            }
        }

        printf("Child exited with %d\n", WEXITSTATUS(exit_status_raw));
    }

    return 0;
}


child.c

#include 
#include "common.h"

int main(int argc, const char * argv[]) {
    printf("This is a time consuming computation\n");
    // ...
    printerr("Sorry. I failed to compute");
    return 127;
}


common.h

#ifndef my_common_h
#define my_common_h

void printerr(const char *message) {
    fprintf(stderr, "%s\n", message);
}

#endif

Solution

-
You could consider terminating with the exit codes from `, especially if the client is expecting them. For instance, if fork() fails, return or call exit() with EX_OSERR.

-
It may be helpful to make the
127 error code a macro with a relevant name, since it's probably not a very commonly-known error code, especially returned from main(). I assume it's being used correctly (based on this post).

-
It would look clearer to put
execlp()'s return value in a conditional, rather than always printing an error afterwards. Since the exec functions only return a value (-1) on error, have the error message displayed only if this value gets returned. Of course, have errno printed out as well.

-
You should also check to see if the child process has terminated from a signal. This can be done by calling
WTERMSIG() and displaying its returned signal value. That can be determined by first checking the return value of WIFEXITED() (before doing the other checks).

Example:

int status;

// do checks with waitpid()

if (WIFEXITED(status))
{
    fprintf(stderr, "the process exited with code %d\n", status);   
    exit(WEXITSTATUS(status));
}
else
{
    fprintf(stderr, "the process died with signal %d\n", WTERMSIG(status));
    exit(EX_OSERR);
}


-
I don't see a need for
printerr() when you can just use perror(), which is already provided in and does more than your function. Regardless, it's a little redundant to put "failed" into the argument. Clients receiving such a message should already be able to see that it's some kind of error, and it's automatically printed to stderr.

-
Instead of just having
default provide an ambiguous error message, have it display the current value of errno with perror()`. Again, your function doesn't do very much.

Code Snippets

int status;

// do checks with waitpid()

if (WIFEXITED(status))
{
    fprintf(stderr, "the process exited with code %d\n", status);   
    exit(WEXITSTATUS(status));
}
else
{
    fprintf(stderr, "the process died with signal %d\n", WTERMSIG(status));
    exit(EX_OSERR);
}

Context

StackExchange Code Review Q#90236, answer score: 5

Revisions (0)

No revisions yet.