Implementation of multiple pipes in C

Question

I'm trying to implement multiple pipes in my shell in C. I found a tutorial on this website and the function I made is based on this example. Here's the function

void executePipes(cmdLine* command, char* userInput) {
    int numPipes = 2 * countPipes(userInput);
    int status;
    int i = 0, j = 0;
    int pipefds[numPipes];

    for(i = 0; i < (numPipes); i += 2)
        pipe(pipefds + i);

    while(command != NULL) {
        if(fork() == 0){

            if(j != 0){
                dup2(pipefds[j - 2], 0);
            }

            if(command->next != NULL){
                dup2(pipefds[j + 1], 1);
            }    

            for(i = 0; i < (numPipes); i++){
                close(pipefds[i]);
            }
            if( execvp(*command->arguments, command->arguments) < 0 ){
                perror(*command->arguments);
                exit(EXIT_FAILURE);
            }
        }

        else{
                if(command != NULL)
                    command = command->next;

                j += 2;
                for(i = 0; i < (numPipes ); i++){
                   close(pipefds[i]);
                }
               while(waitpid(0,0,0) < 0);
        }
    }

}

After executing it and typing a command like for example ls | grep bin, the shell just hangs there and doesn't output any result. I made sure I closed all pipes. But it just hangs there. I thought that it was the waitpid that's was the problem. I removed the waitpid and after executing I get no results. What did I do wrong? Thanks.

Added code:

void runPipedCommands(cmdLine* command, char* userInput) {
    int numPipes = countPipes(userInput);

    int status;
    int i = 0, j = 0;

    pid_t pid;

    int pipefds[2*numPipes];

    for(i = 0; i < 2*(numPipes); i++){
        if(pipe(pipefds + i*2) < 0) {
            perror("pipe");
            exit(EXIT_FAILURE);
        }
    }

    while(command) {
        pid = fork();
        if(pid == 0) {

            //if not first command
            if(j != 0){
                if(dup2(pipefds[(j-1) * 2], 0) < 0){
                    perror(" dup2");///j-2 0 j+1 1
                    exit(EXIT_FAILURE);
                    //printf("j != 0  dup(pipefd[%d], 0])\n", j-2);
                }
            //if not last command
            if(command->next){
                if(dup2(pipefds[j * 2 + 1], 1) < 0){
                    perror("dup2");
                    exit(EXIT_FAILURE);
                }
            }

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

            if( execvp(*command->arguments, command->arguments) < 0 ){
                    perror(*command->arguments);
                    exit(EXIT_FAILURE);
            }
        } else if(pid < 0){
            perror("error");
            exit(EXIT_FAILURE);
        }

        command = command->next;
        j++;
    }
        for(i = 0; i < 2 * numPipes; i++){
            close(pipefds[i]);
            puts("closed pipe in parent");
        }

        while(waitpid(0,0,0) <= 0);

    }

}

Solution 1

I believe the issue here is that your waiting and closing inside the same loop that's creating children. On the first iteration, the child will exec (which will destroy the child program, overwriting it with your first command) and then the parent closes all of its file descriptors and waits for the child to finish before it iterates on to creating the next child. At that point, since the parent has closed all of its pipes, any further children will have nothing to write to or read from. Since you are not checking for the success of your dup2 calls, this is going un-noticed.

If you want to keep the same loop structure, you'll need to make sure the parent only closes the file descriptors that have already been used, but leaves those that haven't alone. Then, after all children have been created, your parent can wait.

EDIT: I mixed up the parent/child in my answer, but the reasoning still holds: the process that goes on to fork again closes all of its copies of the pipes, so any process after the first fork will not have valid file descriptors to read to/write from.

pseudo code, using an array of pipes created up-front:

/* parent creates all needed pipes at the start */
for( i = 0; i < num-pipes; i++ ){
    if( pipe(pipefds + i*2) < 0 ){
        perror and exit
    }
}

commandc = 0
while( command ){
    pid = fork()
    if( pid == 0 ){
        /* child gets input from the previous command,
            if it's not the first command */
        if( not first command ){
            if( dup2(pipefds[(commandc-1)*2], 0) < ){
                perror and exit
            }
        }
        /* child outputs to next command, if it's not
            the last command */
        if( not last command ){
            if( dup2(pipefds[commandc*2+1], 1) < 0 ){
                perror and exit
            }
        }
        close all pipe-fds
        execvp
        perror and exit
    } else if( pid < 0 ){
        perror and exit
    }
    cmd = cmd->next
    commandc++
}

/* parent closes all of its copies at the end */
for( i = 0; i < 2 * num-pipes; i++ ){
    close( pipefds[i] );
}

In this code, the original parent process creates a child for each command and therefore survives the entire ordeal. The children check to see if they should get their input from the previous command and if they should send their output to the next command. Then they close all of their copies of the pipe file descriptors and then exec. The parent doesn't do anything but fork until it's created a child for each command. It then closes all of its copies of the descriptors and can go on to wait.

Creating all of the pipes you need first, and then managing them in the loop, is tricky and requires some array arithmetic. The goal, though, looks like this:

cmd0    cmd1   cmd2   cmd3   cmd4
   pipe0   pipe1  pipe2  pipe3
   [0,1]   [2,3]  [4,5]  [6,7]

Realizing that, at any given time, you only need two sets of pipes (the pipe to the previous command and the pipe to the next command) will simplify your code and make it a little more robust. Ephemient gives pseudo-code for this here. His code is cleaner, because the parent and child do not have to do unnecessary looping to close un-needed file descriptors and because the parent can easily close its copies of the file descriptors immediately after the fork.

As a side note: you should always check the return values of pipe, dup2, fork, and exec.

EDIT 2: typo in pseudo code. OP: num-pipes would be the number of pipes. E.g., "ls | grep foo | sort -r" would have 2 pipes.

Solution 2

Here's the correct functioning code

void runPipedCommands(cmdLine* command, char* userInput) {
    int numPipes = countPipes(userInput);


    int status;
    int i = 0;
    pid_t pid;

    int pipefds[2*numPipes];

    for(i = 0; i < (numPipes); i++){
        if(pipe(pipefds + i*2) < 0) {
            perror("couldn't pipe");
            exit(EXIT_FAILURE);
        }
    }


    int j = 0;
    while(command) {
        pid = fork();
        if(pid == 0) {

            //if not last command
            if(command->next){
                if(dup2(pipefds[j + 1], 1) < 0){
                    perror("dup2");
                    exit(EXIT_FAILURE);
                }
            }

            //if not first command&& j!= 2*numPipes
            if(j != 0 ){
                if(dup2(pipefds[j-2], 0) < 0){
                    perror(" dup2");///j-2 0 j+1 1
                    exit(EXIT_FAILURE);

                }
            }


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

            if( execvp(*command->arguments, command->arguments) < 0 ){
                    perror(*command->arguments);
                    exit(EXIT_FAILURE);
            }
        } else if(pid < 0){
            perror("error");
            exit(EXIT_FAILURE);
        }

        command = command->next;
        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);
}

Solution 3

The (shortened) relevant code is:

    if(fork() == 0){
            // do child stuff here
            ....
    }
    else{
            // do parent stuff here
            if(command != NULL)
                command = command->next;

            j += 2;
            for(i = 0; i < (numPipes ); i++){
               close(pipefds[i]);
            }
           while(waitpid(0,0,0) < 0);
    }

Which means the parent (controlling) process does this:

• fork
• close all pipes
• wait for child process
• next loop / child

But it should be something like this:

• fork
• fork
• fork
• close all pipes (everything should have been duped now)
• wait for childs

Comments

[mkab]

If I understand you well, I should create another fork, then if this fork is 0, I check while(command != NULL). Then I keep the entire code I wrote above in the while command. Am I right?

[A.H.]

The code in my answer is NOT a proposal how to solve the problem. It is a summary of YOUR code with a little emphasis on what actually happens.

[mkab]

Yeah I know. It the same code as mine. I was just trying to understand what you suggested.Please check my edited question. I added some code.

[Kev]

Posting your email address for the OP to request your code is not really the way to answer questions here. Feel free to expand your answer though.

[mkab]

Thanks for your help. I might sound desperate but can you give me a pseudo code that can mimic what you believe the issue is? I tried everything I could but I still have a memory leak somewhere. I just don't see where the problem is. Thanks

[mkab]

Thanks for the pseudo code. In your code does num-pipe mean number of pipes? Because if it's it, following your pseudo code would give me bad file descriptors.

[mkab]

I tried implementing your code. Please check my edited question. I added the code.

[Christopher Neylan]

I implemented it to confirm it works. Sorry about the index typos. Again, it's easier to just keep two sets of pipes at all times and rotate them as you loop.

[mkab]

No problem :). And just to make sure, in my case if i want to declare pipe file descriptors, it would be int pipefds[2*numPipes] right?

[Christopher Neylan]

Right. You want two integers for each pipe: one for output and one for input. So if you need 3 pipes, you'll need 6 integers.

[Christopher Neylan]

Study my pipe diagram. In it, there are 5 commands, which need 4 pipes. It shows the "pipe" array and the indexes that each pair of commands will need to use.

[mkab]

Cool and nice ...I get it. And in your code why doesn't the parent wait for the children to finish their jobs before exiting?

[Christopher Neylan]

No reason. It should wait on all of its children to finish.

[mkab]

I'm getting an infinite loop after implementing it. I edited my question to put the code I implemented. Please check it.

[Christopher Neylan]

You're using waitpid incorrectly. Consider just using wait(0). See linux.die.net/man/2/waitpid

[mkab]

I used wait(0) but it didn't solve the problem. Instead I get a bad file descriptor from perror(dup2)

[mkab]

It did solve the problem of the parent waiting forever but now I get a bad file descriptor from perror(dup2)

[mkab]

Found my error. I nested incorrectly my braces and wait(0) isn't enough. You have to put it in a for loop so that the parent waits for its the children. Thanks for your help though. Pointed me in the right direction. Cheers

[Knu]

Is something missing in this condition? if(j != 0 ){

[mkab]

@Knu: It might be too late but no, nothing is missing in that condition.

[wizzwizz4]

@mkab Are you sure? You have j!= 2*numPipes in the comment above the statement.

[andrzej1_1]

@ChristopherNeylan I cannot understand, why all pipes are closed inside loop instead of duplicated only and why parent is closing all pipes additionally. Could you explain this?