Trying to build a shell with pipelines and background

Question

Hi I am trying to recreate a shell and I am having two major problems:

1. After executing one single command it finishes the program

2. The pipeline doesn´t work

Here is the part of the code that deals with the pipes, redirections ...

int pfd[2];
if (pipe(pfd) < 0) exit(-1);
for (int i = 0; i < cmd.pipes; i++) {
    pid_t pid;
    pid = fork();
    int fd;
    if (pid < 0) exit(-1);
    else if (pid == 0) {
        close(pfd[0]);
        dup2(pfd[1], STDOUT_FILENO);
        close(pfd[1]);
        if (cmd.filev[0] != NULL && i == 0) {
            fd = open(cmd.filev[0], O_RDONLY, 0);
            dup2(fd, STDIN_FILENO);
            close(fd);
        }
        if (cmd.filev[2] != NULL) {
            fd = creat(cmd.filev[2], 0644);
            dup2(fd, STDERR_FILENO);
            close(fd);
        }
        if (execvp(cmd.argv[i][0], cmd.argv[i]) < 0)
            levenshtein(cmd.argv[i][0], commands);
    } else if (pid > 0) {
        if (cmd.bg > 0) wait(NULL);
        close(pfd[1]);
        dup2(pfd[0], STDIN_FILENO);
        close(pfd[0]);
        if (cmd.filev[1] != NULL && i == (cmd.pipes - 1)) {
            fd = creat(cmd.filev[1], 0644);
            dup2(fd, STDOUT_FILENO);
            close(fd);
        }
        if (cmd.filev[2] != NULL) {
            fd = creat(cmd.filev[2], 0644);
            dup2(fd, STDERR_FILENO);
            close(fd);
        }
        if (execvp(cmd.argv[i][0], cmd.argv[i]) < 0)
            levenshtein(cmd.argv[i][0], commands);
    }
}

PD: Levenshtein is a function to deal when there is a misspell by the user

It might help to see the `struct` declaration for `cmd` and some of the other related code. Also, for an example, see my answer: https://stackoverflow.com/questions/52823093/fd-leak-custom-shell/52825582#52825582 — Craig Estey, Jul 13 '20 at 16:35
cmd is in fact a structure where I save the input parsed (argv (command), pipes (number of commands), bg (background signal) and filev[3] (redirections)) — alvarogv, Jul 13 '20 at 16:43
If there are N commands in the pipeline, you will need to fork N times. If the parent is used to run one of the processes, then it will terminate prematurely. — William Pursell, Jul 13 '20 at 16:54

Craig Estey · Answer 1 · 2020-07-14T15:35:36.963

This needs a bit of refactoring ...

If there are N pipeline stages, we need N-1 separate pipe calls. The code only has one.

Each pipeline stage can have its own/private stderr diversion (e.g.):

cmd0 | cmd1 2>cmd1_stderr | cmd2 2>cmd2_stderr | cmd3

But, the code assumes all stderr will be the same.

And, it assumes that it has to open stderr [at all]. The child should only open stderr if we have something like above. Otherwise, each child should inherit the parent's stderr [and do nothing].

The parent process is doing things that only the child should do (e.g.): change stdin/stdout/stderr and execute the command. It should mostly just facilitate the pipe.

The parent is doing a wait in the creation/fork loop [for each child]. This causes the parent to block at each step.

The parent should only do the wait in a second loop and wait for all children at once.

Because your struct definition for cmd was not posted, I had to guess a bit as to intent. But, I think you should have an array of the structs, one for each command, rather than putting all arguments for all commands in a single struct.

I've created two versions of the code. One with annotations for the bugs. A second that is cleaned up and restructured. Both are untested but should give you some ideas.

Here's the annotated version:

// NOTE/BUG: each command in the pipeline can have its own/private
// stderr diversion
#if 0
struct cmd {
    int pipes;
    char *filev[3];
    char **argv[MAXCMD][MAXARG];
};
#else
struct cmd {
    char *stderr;
    char *argv[100];
};
#endif

// NOTE/BUG: we need separate filev [and argv] for each pipeline stage
// so we need an _array_ of structs
int cmdcnt;
struct cmd cmdlist[30];

void
pipeline(void)
{
    int pfd[2];

// NOTE/BUG: this only creates a single pipe -- we need N-1 pipes
#if 0
    if (pipe(pfd) < 0)
        exit(-1);
#endif

// NOTE/BUG: if child does _not_ have a private stderr it should just
// use the parent's stderr _unchanged_

    for (int i = 0; i < cmdcnt; i++) {
        pid_t pid;

// NOTE/BUG: here is the correct place to create the pipe
        pid = fork();
        int fd;

        if (pid < 0)
            exit(-1);

        char **argv = cmd->argv;
        char **filev = cmd->filev;

        // child process
        if (pid == 0) {
            close(pfd[0]);
            dup2(pfd[1], STDOUT_FILENO);
            close(pfd[1]);

// NOTE/BUG: this does _not_ connect the input of cmd[N] to cmd[N-1]
#if 0
            if (filev[0] != NULL && i == 0) {
                fd = open(filev[0], O_RDONLY, 0);
                dup2(fd, STDIN_FILENO);
                close(fd);
            }
#endif

            if (filev[2] != NULL) {
                fd = creat(filev[2], 0644);
                dup2(fd, STDERR_FILENO);
                close(fd);
            }

            if (execvp(cmd->argv[i][0], cmd->argv[i]) < 0)
                levenshtein(cmd->argv[i][0], commands);
        }

        // parent process
        if (pid > 0) {
// NOTE/BUG: parent should _not_ wait in the middle of the creation
// loop
            if (cmd->bg > 0)
                wait(NULL);

// NOTE/BUG: _parent_ should _not_ change its stdin/stderr/stdout

            close(pfd[1]);
            dup2(pfd[0], STDIN_FILENO);
            close(pfd[0]);

            if (filev[1] != NULL && i == (cmd->pipes - 1)) {
                fd = creat(filev[1], 0644);
                dup2(fd, STDOUT_FILENO);
                close(fd);
            }

            if (filev[2] != NULL) {
                fd = creat(filev[2], 0644);
                dup2(fd, STDERR_FILENO);
                close(fd);
            }

// NOTE/BUG: _parent_ should _not_ execute the command
            if (execvp(cmd->argv[i][0], cmd->argv[i]) < 0)
                levenshtein(cmd->argv[i][0], commands);
        }
    }
}

Here's the refactored version:

#include <stdlib.h>
#include <unistd.h>
#include <fcntl.h>
#include <sys/wait.h>

struct cmd {
    char *stderr;
    char *argv[100];
};

char *filev[3];

#define CLOSEME(_fd) \
    do { \
        if (_fd < 0) \
            break; \
        close(_fd); \
        _fd = -1; \
    } while (0)

int run_in_background;
int cmdcnt;
struct cmd cmdlist[30];

int
pipeline(void)
{
    int pfd[2] = { -1, -1 };
    struct cmd *cmd;
    char *file;
    int oldfd;
    pid_t pid;
    pid_t lastpid = -1;

    // open the common stderr
    int errfd = -1;
    if (filev[2] != NULL)
        errfd = open(filev[2],O_APPEND | O_CREAT);

    // start all commands
    for (int i = 0; i < cmdcnt; i++) {
        int iam_first = (i == 0);
        int iam_last = (i == (cmdcnt - 1));

        cmd = &cmdlist[i];

        // get previous stage pipe descriptor
        oldfd = pfd[0];

        // create the pipe to the next stage
        if (! iam_last) {
            if (pipe(pfd) < 0)
                exit(1);
        }

        pid = fork();
        lastpid = pid;

        int fd;

        if (pid < 0)
            exit(-1);

        char **argv = cmd->argv;

        // parent process
        if (pid > 0) {
            CLOSEME(pfd[1]);
            continue;
        }

        // child process ...

        // open stdin for _first_ command
        fd = -1;
        if (iam_first) {
            file = filev[0];
            if (file != NULL) {
                fd = open(file, O_RDONLY, 0);
            }
        }

        // connect stdin to previous stage pipe
        else {
            fd = oldfd;
            oldfd = -1;
        }

        // connect stdin to correct source
        if (fd >= 0) {
            dup2(fd, STDIN_FILENO);
            close(fd);
        }
        CLOSEME(oldfd);

        // connect to stderr
        file = cmd->stderr;
        if (file != NULL) {
            fd = creat(file, 0644);
            dup2(fd, STDERR_FILENO);
            close(fd);
        }
        else {
            if (errfd >= 0)
                dup2(errfd, STDERR_FILENO);
        }
        CLOSEME(errfd);

        // connect stdout
        // NOTE: does _not_ handle ">> outf" [only does "> outf"]
        fd = -1;
        if (iam_last) {
            file = filev[1];
            if (file != NULL) {
                fd = open(file, O_WRONLY | O_CREAT, 0644);
                dup2(fd, STDOUT_FILENO);
                close(fd);
            }
        }

        // execute the command
        execvp(argv[0], argv);
        exit(9);
    }

    CLOSEME(errfd);

    int status;
    int last_status = 0;

    // parent waits for all pipeline stages to complete
    if (! run_in_background) {
        while (1) {
            pid = wait(&status);
            if (pid <= 0)
                break;
            if (pid == lastpid) {
                last_status = status;
                break;
            }
        }
    }

    return last_status;
}

Trying to build a shell with pipelines and background

1 Answers1