How to properly wait for foreground/background processes in my own shell in C?

Question

In this previous question I posted most of my own shell code. My next step is to implement foreground and background process execution and properly wait for them to terminate so they don't stay as "zombies".

Before adding the possibility to run them in the background, all processes were running in the foreground. And for that, I simply called wait(NULL) after executing any process with execvp(). Now, I check for the '&' character as the last argument and if it's there, run the process in the background by not calling wait(NULL) and the process can run happily in the background will I'm returned to my shell.

This is all working properly (I think), the problem now, is that I also need to call wait() (or waitpid() ?) somehow so that the background process doesn't remain "zombie". That's my problem, I'm not sure how to do that...

I believe I have to handle SIGCHLD and do something there, but I have yet to fully understand when the SIGCHLD signal is sent because I tried to also add wait(NULL) to childSignalHandler() but it didn't work because as soon as I executed a process in the background, the childSignalHandler() function was called and consequently, the wait(NULL), meaning I couldn't do anything with my shell until the "background" process finished. Which wasn't running on the background anymore because of the wait in the signal handler.

What am I missing in all this?

One last thing, part of this exercise I also need to print the changes of the processes status, like process termination. So, any insight on that is also really appreciated.

This is my full code at the moment:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <wait.h>
#include <signal.h>
#include <sys/types.h>

#include "data.h" // Boolean typedef and true/false macros


void childSignalHandler(int signum) {
    //
}

int main(int argc, char **argv) {
    char bBuffer[BUFSIZ], *pArgs[10], *aPtr = NULL, *sPtr;
    bool background;
    ssize_t rBytes;
    int aCount;
    pid_t pid;

    //signal(SIGINT, SIG_IGN);

    signal(SIGCHLD, childSignalHandler);

    while(1) {
        write(1, "\e[1;31mmyBash \e[1;32m# \e[0m", 27);
        rBytes = read(0, bBuffer, BUFSIZ-1);

        if(rBytes == -1) {
            perror("read");
            exit(1);
        }

        bBuffer[rBytes-1] = '\0';

        if(!strcasecmp(bBuffer, "exit")) {
            exit(0);
        }

        sPtr = bBuffer;
        aCount = 0;

        do {
            aPtr = strsep(&sPtr, " ");
            pArgs[aCount++] = aPtr;
        } while(aPtr);

        background = FALSE;

        if(!strcmp(pArgs[aCount-2], "&")) {
            pArgs[aCount-2] = NULL;
            background = TRUE;
        }

        if(strlen(pArgs[0]) > 1) {
            pid = fork();

            if(pid == -1) {
                perror("fork");
                exit(1);
            }

            if(pid == 0) {
                execvp(pArgs[0], pArgs);
                exit(0);
            }

            if(!background) {
                wait(NULL);
            }
        }
    }

    return 0;
}

Majunga

Answer 1

There are various options to waitpid() to help you (quotes from the POSIX standard):

WCONTINUED

The waitpid() function shall report the status of any continued child process specified by pid whose status has not been reported since it continued from a job control stop.

WNOHANG

The waitpid() function shall not suspend execution of the calling thread if status is not immediately available for one of the child processes specified by pid.

In particular, WNOHANG will allow you to see whether there are any corpses to collect without causing your process to block waiting for a corpse.

If the calling process has SA_NOCLDWAIT set or has SIGCHLD set to SIG_IGN, and the process has no unwaited-for children that were transformed into zombie processes, the calling thread shall block until all of the children of the process containing the calling thread terminate, and wait() and waitpid() shall fail and set errno to [ECHILD].

You probably don't want to be ignoring SIGCHLD, etc, and your signal handler should probably be setting a flag to tell your main loop "Oops; there's dead child - go collect that corpse!".

The SIGCONT and SIGSTOP signals will also be of relevance to you - they are used to restart and stop a child process, respectively (in this context, at any rate).

I'd recommend looking at Rochkind's book or Stevens' book - they cover these issues in detail.

Jabiru

Answer 2

This should get you started. The major difference is that I got rid of the child handler and added waitpid in the main loop with some feedback. Tested and working, but obviously needs more TLC.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <strings.h>
#include <unistd.h>
#include <wait.h>
#include <signal.h>
#include <sys/types.h>

int main(int argc, char **argv) {
        char bBuffer[BUFSIZ], *pArgs[10], *aPtr = NULL, *sPtr;
        int background;
        ssize_t rBytes;
        int aCount;
        pid_t pid;
        int status;
        while(1) {
                pid = waitpid(-1, &status, WNOHANG);
                if (pid > 0)
                        printf("waitpid reaped child pid %d\n", pid);
                write(1, "\e[1;31mmyBash \e[1;32m# \e[0m", 27);
                rBytes = read(0, bBuffer, BUFSIZ-1);
                if(rBytes == -1) {
                        perror("read");
                        exit(1);
                }
                bBuffer[rBytes-1] = '\0';
                if(!strcasecmp(bBuffer, "exit")) 
                        exit(0);
                sPtr = bBuffer;
                aCount = 0;
                do {
                        aPtr = strsep(&sPtr, " ");
                        pArgs[aCount++] = aPtr;
                } while(aPtr);
                background = (strcmp(pArgs[aCount-2], "&") == 0);
                if (background)
                        pArgs[aCount-2] = NULL;
                if (strlen(pArgs[0]) > 1) {
                        pid = fork();
                        if (pid == -1) {
                                perror("fork");
                                exit(1);
                        } else if (pid == 0) {
                                execvp(pArgs[0], pArgs);
                                exit(1);
                        } else if (!background) {
                                pid = waitpid(pid, &status, 0);
                                if (pid > 0)
                                        printf("waitpid reaped child pid %d\n", pid);
                        }
                }
        }
        return 0;
}

EDIT: Adding back in signal handling isn't difficult with waitpid() using WNOHANG. It's as simple as moving the waitpid() stuff from the top of the loop into the signal handler. You should be aware of two things, though:

First, even "foreground" processes will send SIGCHLD. Since there can be only one foreground process you can simply store the foreground pid (parent's return value from fork()) in a variable visible to the signal handler if you want to do special handling of foreground vs. background.

Second, you're currently doing blocking I/O on standard input (the read() at main loop top). You are extremely likely to be blocked on read() when SIGCHLD occurs, resulting in an interrupted system call. Depending on OS it may restart the system call automatically, or it may send a signal that you must handle.

Tella

Thanks, but I really need to use the signal handler, it's part of the exercise. – Majunga

I just saw your edit... I did put the waitpid() into the signal handler but that I have the problem you describe in your second paragraph. I just didn't want to use global variables to fix the problem but I'm not sure how to fix it without them... – Majunga

Avoiding global variables is good, but communicating with a signal handler is one of the cases where they're needed. – Tella

Answer 3

You may use:

if(!background)
    pause();

This way, the process blocks until it receives the SIGCHLD signal, and the signal handler will do the wait stuff.

Villainous

What if the child signal comes between checking background and calling pause()? There's a race condition here. – Schuh

Answer 4

Instead of using a global variable, I thought of a different solution:

if(!background) {
    signal(SIGCHLD, NULL);

    waitpid(pid, NULL, 0);

    signal(SIGCHLD, childSignalHandler);
}

If I'm running a foreground process "delete" the handler for SIGCHLD so it doesn't get called. Then, after waitpid(), set the handler again. This way, only the background processes will be handled.

Do you think there's anything wrong with this solution?

Majunga

You have introduced a race condition where a background process exits but no signal handler is installed, resulting in an unreaped child and eventually a zombie. Don't add such problems to your program just to avoid a global variable, just because you heard they were bad. – Tella

But I'm still not seeing a how to implement a global variable for this and also a avoid a race condition. The way I see it, adding a global variable to handle this situation will still create a race condition. Care to provide an example where it doesn't introduce a race condition? – Majunga

Note that the second argument to signal() should be a pointer to a signal handler function, or SIG_IGN or SIG_DFL; NULL is not a valid option, even though SIG_IGN is often a null function pointer. You should probably capture the return value from the first signal() and reinstate it in the second call. If other signals the SIGCHLD arrive, waitpid() could return early with EINTR. Using sigaction() instead of signal() would allow you to control which signals are allowed in. – Jabiru