Running shell commands in a pipeline

Question

I'm writing a shell in C for Linux and BSD. My code was unreadable before but I did a rewrite and now it is much more readable. The purpose of the code is to create a command pipeline and execute it.

int run_cmd(const char *cmd) {
    struct str_list *chunks = list_split(cmd, '|');
    struct pipeline *pipe = malloc(chunks->pipes * sizeof *pipe);
    pipe->data = malloc(sizeof(char *));
    int i = 0;
    for (i = 0; i < chunks->pipes; i++) {
        pipe[i].data = malloc(sizeof(char *) * 3);
        for (int j = 0; j < 1; j++) {
            pipe[i].data[j] = strdup(chunks[i].argv[j]);
        }
    }
    pipe->size = chunks->pipes;
    int status = execute_pipeline(pipe);
    return status;
}

The above code makes the correct output and doesn't crash, but maybe it can be refactored even more. What was suggested was that I create structs:

struct str_list {
    char *name;
    int size;
    int pipes;
    char **argv;

};
struct pipeline {
    char *name;
    int size;
    char **data;
};

The helper functions: The first one takes a command and splits it first in pipelines and then into arguments and returns a struct. The second helper function is executing the pipeline.

/* TODO: modify str_split to do the copying of its input string if it needs to (e.g. if it uses strtok on it), and return a struct that has the number of "chunks" it split out and the list of chunks. */
struct str_list *list_split(char *a_str, const char a_delim) {
    char **result = 0;


    size_t count = 0;
    char *tmp = a_str;
    char *last_comma = 0;

    size_t count2 = 0;
    char *tmp2 = a_str;


    char delim[2];
    delim[0] = a_delim;
    delim[1] = 0;
    struct str_list *chunks = NULL;
    /* Count how many elements will be extracted. */
    while (*tmp) {
        if (a_delim == *tmp) {
            count++;
            last_comma = tmp;
        }
        tmp++;

    }

    /* Add space for trailing token. */
    count += last_comma < (a_str + strlen(a_str) - 1);

    count++;
    result = malloc(sizeof(char *) * count);
    chunks = malloc(count * sizeof *chunks);

    if (result == NULL) {
        printf("Error allocating memory!\n"); //print an error message
        return chunks;; //return with failure
    }

    if (result) {
        size_t idx = 0;
        char *token = strtok(a_str, delim);
        int i = 0;
        while (token) {
            assert(idx < count);
            *(result + idx++) = strdup(token); /* memory leak! how to free() */;
            token = strtok(0, delim);;
        }
        assert(idx == count - 1);
        *(result + idx) = 0;
    }
    chunks->argv = malloc(chunks->size * sizeof(char *) * 1);//alloc_argv((unsigned) chunks->size);
    int i = 0;

    for (i = 0; i < count - 1; i++) {
        tmp2 = result[i];
        chunks[i].size = 0;
        while (*tmp2) {
            if (' ' == *tmp2) {
                count2++;
            }
            tmp2++;
        }
        //chunks[i].size = (unsigned) count2 + 1;
        int k = 0;
        char *token = strtok(result[i], " ");
        chunks[i].argv = alloc_argv(BUFFER_SIZE);
        while (token) {
            chunks[i].argv[k] = malloc(sizeof(char *));
            chunks[i].argv[k] = strdup(token);
            token = strtok(0, " ");
            k++;
        }
        chunks[i].size = k;
    }
    chunks->pipes = i;
    return chunks;
}





int execute_pipeline(struct pipeline *pipe) {
    int status = 0;
    pid_t pid;
    pid = fork();
    if (pid < 0) {
         perror("fork failed");
         return -1;
    }
    /* If we are the child process, then go execute the string.*/
    if (pid == 0) {
        /* spawn(cmd);*/
        fork_pipeline(pipe->size, pipe);
    }
    /*
     * We are the parent process.
     * Wait for the child to complete.
     */

    while (((pid = waitpid(pid, &status, 0)) < 0) && (errno == EINTR));
    if (pid < 0) {
        fprintf(stderr, "Error from waitpid: %s", strerror(errno));

        return -1;
    }
    if (WIFSIGNALED(status)) {
        fprintf(stderr, "pid %ld: killed by signal %d\n",
                (long) pid, WTERMSIG(status));


        return -1;
    }


    return WEXITSTATUS(status);
    return 1;

}


/* Helper function that forks pipes */
void fork_pipeline(int n, struct pipeline *pipel) {
    int i;
    int in = 0;
    int fd[2];
    for (i = 0; i < n - 1; ++i) {

        if (pipe(fd) == -1) {
            err_syserr("Failed creating pipe");
        }

        spawn_proc2(in, fd[1], pipel + i);
        close(fd[1]);
        in = fd[0];
    }
    if (dup2(in, 0) < 0) {
        err_syserr("dup2() failed on stdin for %s: ", pipel[i].data[0]);
    }
    /*fprintf(stderr, "%d: executing %s\n", (int) getpid(), cmd[i].argv[0]);*/
    fprintf(stderr, "\n");

    execvp(pipel[i].data[0], pipel[i].data);
    err_syserr("failed to execute %s: ", pipel[i].data[0]);
}


/* Helper function that spawns processes */
int spawn_proc2(int in, int out, struct pipeline *cmd) {
    pid_t pid;
    fflush(NULL);
    pid = fork();
    if (pid == 0) {
        if (in != 0) {
            if (dup2(in, 0) < 0)
                err_syserr("dup2() failed on stdin for %s: ", cmd->data[0]);
            close(in);
        }
        if (out != 1) {
            if (dup2(out, 1) < 0)
                err_syserr("dup2() failed on stdout for %s: ", cmd->data[0]);
            close(out);
        }
        /*fprintf(stderr, "%d: executing %s\n", (int) getpid(), cmd->argv[0]);*/
        fprintf(stderr, "[%d]\n", (int) getpid());
        execvp(cmd->data[0], cmd->data);
        err_syserr("failed to execute %s: ", cmd->data[0]);
    }
    else if (pid < 0) {
        err_syserr("fork failed: ");
    } else {
        /* printf("** we are the parent ***"); */
    }
    return pid;
}

The code works:

$ ls -al|cat|grep -E CMake
ls -al|cat|grep -E CMake
-rw-rw-r-- 1 dac dac  12180 maj 11 12:40 CMakeCache.txt
drwxrwxr-x 6 dac dac   4096 maj 12 12:03 CMakeFiles

Answer 1

Just looking at list_split:

1. There's no specification for this function. Without a specification, it's hard to review it: how am I to know if the code is correct unless I know what it's supposed to do?

2. There's no specifications for the str_list type. What is str_list.size the size of? What does str_list.pipes count? What is str_list.name? (It doesn't seem to be used anywhere.)

3. Error checking is far from complete: the code calls functions (malloc and strdup) that might fail, but in most cases does not check the results.

4. There's no way for the caller to tell if str_list succeeded or failed. For example, if the allocation of result fails, it returns chunks, which might, for all we know, be a valid pointer. It would be better to arrange the code so that there is a return value indicating success or failure.

5. Memory is allocated but never freed, which will cause the process size to grow without bound, eventually leading to some kind of failure.

6. The splitting just proceeds character-by-character without regard to syntax. This means that a command cannot contain a vertical bar, and an argument cannot contain a space. But real shells have quoting, so that we can write pipelines like:

   echo "This argument has | and spaces" | grep -c "|"
   

   Obviously you're just getting started and want to keep things simple, but it would be a good idea to think ahead about how you are going to solve this problem.

7. Similarly, your implementation assumes that a pipeline is a list of commands, each of which is a list of arguments. But real shells have more structure than this: they have compound commands:

   { echo 1; echo 2; echo 3; } | while read X; do printf "%02d" $X; done
   

   Again, it's worth thinking ahead about how you are going to implement compound commands.