3
\$\begingroup\$

The following program uses a parent program to determine the PIDs of multiple child processes. A function is called to return them to a dynamic vector (with the children's pids, but also the elements in the vector)

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

int* createChild(int* array, int* nrChildren) {

        if(*nrChildren > 0) {
                array = (int*)realloc(array, (*nrChildren + 1) * sizeof(int));
                if(array == NULL) {
                        perror("Realloc failed!\n");
                        exit(EXIT_FAILURE);
                }
        }

        int pid = fork();
        if(pid < 0) {
                perror("No child process is created!\n");
                exit(EXIT_FAILURE);
        }
        else if(pid > 0){
                array[*nrChildren] = pid;
                *nrChildren += 1;
        }
        else while(1);

        return array;
}

int main(void) {

        int i, nrChildren = 0;
        int* array;
        array = (int*)malloc(sizeof(int));
        for(i = 1; i <= 5; i++)
                createChild(array, &nrChildren);

        for(i = 0; i < nrChildren; i++) {
                printf("Child: %d\n", array[i]);
                int killed = kill(array[i], SIGKILL);
                if(killed == -1) {
                        perror("kill\n");
                        exit(EXIT_FAILURE);
                }
        }

        printf("Total children: %d\n", nrChildren);

        free(array);
        return 0;
}

Please criticize my implementation?

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Limit the responsibilites of createChild()

The function createChild() does many things; it resizes an array, forks a new process, and runs the child process (even though it's just while(1);). Try to follow the single-responsibility principle, and limit what createChild() does to a minimum. In particular, it should just create the child process as its name implies. Instead of dealing with an array, it can just return the PID of the child, and let the caller deal with it. And instead of actually containing the code that the child process will run, pass a function pointer as an argument to createChild(), so you can use it to start any function in a child process. This is what it could look like:

pid_t createChild(int (*function)()) {
    pid_t pid = fork();

    if (pid == -1) {
        perror("Could not create child process");
        exit(EXIT_FAILURE);
    } else if (pid == 0) {
        return pid;
    }

    function();
    exit(EXIT_SUCCESS);
}

Since you already know how many children you are creating in main(), you also don't need a dynamically allocated array:

static void waitForever(void) {
    while(1);
}

int main(void) {
    const size_t nrChildren = 5;
    pid_t pids[nrChildren];

    for (size_t i = 0; i < nrChildren; i++)
        pids[i] = createChild(waitForever);

    ...
}

If you still want to use a dynamic vector, write a function whose sole responsibility is adding an item to a vector:

void addToVector(pid_t** array, size_t* size, pid_t value) {
    *array = realloc(*array, (*size + 1) * sizeof(**array));

    if (*array == NULL) {
        perror("Could not resize array");
        exit(EXIT_FAILURE);
    }

    (*array)[*size] = value;
    *size += 1;
}

And then use it like so:

size_t nrChildren = 0;
pid_t* pids = NULL;

for (size_t i = 0; i < 5; i++)
    addToVector(&pids, &nrChildren, createChild(waitForever));

Use the correct types

fork() doesn't return an int, it returns a pid_t. Make sure you use the right types; while on your platform it might be that pid_t is actually equivalent to an int, that might not be true for others, depending on the operating system and whether it is a 32 or 64-bit platform.

For counting and array indices, prefer using size_t. It is guaranteed to be large enough to cover all possible array sizes, whereas int might not.

Bug in reallocating the array

When you call realloc() on array, you are only updating the local copy of that variable, the one in main() stays unchanged. You return the new value of array, but you are never using the return value in main(). If the code worked at all, it's just pure luck. Either write:

array = createChild(array, &nrChildren);

Or see my example addToVector() that takes a pointer to pointer argument. But even better is to not use dynamic arrays if they are not necessary.

Only -1 indicates an error in most POSIX functions

If you read the manpage of fork(), you will note that it says that the return value -1 indicates an error, it doesn't mention other negative values. While it might be that PIDs can never be negative on your platform, it's best to not make assumptions.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.