custom shell multiple pipes succeed, but no output to stdout

Question

I am creating a custom shell and currently working on getting multiple piping to work. Eg, ls -al | wc -l returns the number of all file directories in current directory. I am closely following the "solution code" at this link.

Here is my implementation, with very similar command input structure:

// every command is a 
// struct command
// with their arguments attached
struct command
{
    char **cmd; // argument list for execvp(eg: {"ls", "-al"})
    int numArgs; // number of total arguments in current line 
};

// my implementation of multiple pipes based on link supplied with slight modifications
void runPipedCommands(struct command *commands)
{
    // this code just counds the number of pipes in commands
    int numPipes = 0;
    for (int i = 0; i < commands->numArgs; i++)
    {
        if (!strcmp(commands[i].cmd[0], "|"))
            numPipes++;
    }
    printf("number of pipes: %d\n", numPipes);

    int status;
    int i = 0;
    pid_t pid;

    int pipefds[2 * numPipes];
    // create the pipes
    for (i = 0; i < (numPipes); i++)
    {
        if (pipe(pipefds + i * 2) < 0)
        {
            perror("couldn't pipe");
            exit(EXIT_FAILURE);
        }
    }

    int j = 0;
    int c = 0;
    while (commands[c].cmd)
    {
        // skip the pipe operators
        if (!strcmp(commands[c].cmd[0], "|"))
            c++;
        pid = fork();
        if (pid == 0)
        {

            printf("cmd to execute: %s\n", commands[c].cmd[0]);
            //if not last command
            if (commands[c + 1].cmd[0])
            {
                // everyone even fd gets this, set stdout to write
                // end of pipe
                if (dup2(pipefds[j + 1], STDOUT_FILENO) < 0)
                {
                    perror("dup2");
                    exit(EXIT_FAILURE);
                }
            }
            else
            {
                exit(EXIT_SUCCESS);
            }

            //if not first command&& j!= 2*numPipes
            if (j != 0)
            {
                // every odd fd gets this, set stdin to
                // read end of pipe
                if (dup2(pipefds[j - 2], STDIN_FILENO) < 0)
                {
                    perror(" dup2");
                    exit(EXIT_FAILURE);
                }
            }

            // close dup2ed fds
            for (i = 0; i < 2 * numPipes; i++)
            {
                close(pipefds[i]);
            }
            // execvp
            if (execvp(commands[c].cmd[0], commands[c].cmd) < 0)
            {
                perror(commands[c].cmd[0]);
                exit(EXIT_FAILURE);
            }
        }
        else if (pid < 0)
        {
            perror("error");
            exit(EXIT_FAILURE);
        }

        c++;
        j += 2;
    }
    /**Parent closes the pipes and wait for children*/

    for (i = 0; i < 2 * numPipes; i++)
    {
        close(pipefds[i]);
    }

    for (i = 0; i < numPipes + 1; i++)
        wait(&status);
}

// my main function with my test input:
int main(int argc, char *argv[])
{
    // my parseW function simply returns a list of struct commands
    struct command *results = parseW("ls -al | wc -l");

    runPipedCommands(results);
    return 0;
}

The above code with parseW omitted results in a terminal output of:

Everything looks correct in the execution of the pipes, and I've gone over the file descriptors (there are only 2 to account for in my specific test case). I'm not sure what I'm doing wrong.

score 2 · Answer 1 · answered Nov 10 '20 at 17:05

Without posting parseW(), the content of a commands[] array is unknown.
Within runPipedCommands() while loop, there are 3 areas that need revisions:

Provided a commands[x].cmd[] may privately contain a pipe, there's nothing to execute and no need to fork() and exit; instead add a continue :

// skip the pipe operators
if (!strcmp(commands[c].cmd[0], "|")) {
    c++;
    continue;
}

After a fork(), the child process tries to test the next element of commands[], assumes that member .cmd is non-NULL and deferences with .cmd[0]; provided the last element of commands[] contains a NULL .cmd, the child would crash here:

if (commands[c + 1].cmd[0])

instead, update the test without the dereference:

if (commands[c + 1].cmd)

Related with the added continue above, when this updated .cmd test is false, there's no need to exit and prevent the child from reaching the execvp(); delete this block:

else {
    exit(EXIT_SUCCESS);
}

Also: when the parent calls wait(), the status can be checked with macros and indicate whether the child terminated normally, see a wait manual

thank you for the revisions, I had already implemented revisions 2 and 3, but I just revised my code for your first point. The pipes now work! I am now thinking high level how I might support redirection commands before the pipe. For eg. how can I support commands like `program < inputFile | someOtherProgram`? I was thinking that I could use `runPipedCommands` as the outermost loop, and before I exec, I would run through all commands on the left of the pipe, and redirect it's stdout to the pipe. Would this work? — Aaron Li, Nov 10 '20 at 17:15
General idea - instead of building additional elements of `commands[]` with `.cmd` containing only a pipe or redirect, you might `#define` some symbols to unique bits like 0x1, 0x2, 0x4, 0x8 indicating pipe or redirect out/in; then add a member to your struct like an `unsigned int`, set bits while parsing, and test those bits in `runPipedCommands()` — Milag, Nov 10 '20 at 17:42

custom shell multiple pipes succeed, but no output to stdout

1 Answers1