1

I'm trying to implement multiple piping using a tutorial I got from this website. I seem to get a bad file descriptor error after executing the function that takes care of multiple piping. When I'm duping for the first time it sends me this error. Here's the code:

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

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

    pid_t pid;

    int pipefds[2*numPipes];

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

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

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

                }

            if(command->next){
                printf(
                if(dup2(pipefds[j + 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]);
            printf("in parent: closed pipe[%d]\n", i);
        }
        wait(0);
    }
}

Maybe there's a leakage somewhere or it can't find the descriptor. I don't seem to know where the problem is. What have I done wrong? Thanks.

Edited code:

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

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

    pid_t pid;

    int pipefds[2*numPipes];

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

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

                }
            }

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

            for(i = 0; i < 2*numPipes; i++){
                    close(pipefds[i]);
                   printf("in child: closed pipe[%d]\n", 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;
    }
        for(i = 0; i < 2 * numPipes; i++){
            close(pipefds[i]);
            printf("in parent: closed pipe[%d]\n", i);
        }   
           wait(0);
    }
mkab
  • 933
  • 4
  • 16
  • 31

2 Answers2

2

Ok, first, something that's a little odd -- your nesting does not line up with your braces. if (j != 0) and if(command->next) look like the same "level" but the actual braces tell a different story:

Copy-and-paste:

        if(j != 0){
            if(dup2(pipefds[j-2], 0) < 0){
                perror(" dup2");///j-2 0 j+1 1
                exit(EXIT_FAILURE);
            }

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

Re-indented to reflect the braces:

if (j != 0) {
    if (dup2(pipefds[j - 2], 0) < 0) {
        perror(" dup2");    ///j-2 0 j+1 1
        exit(EXIT_FAILURE);
    }

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

Please ask your IDE, editor, or indent(1) to re-indent your code to reflect the actual syntax of your code, so that you're not confused by misleading whitespace.

Second, I think you changed the j+=2 from a j++ in an earlier iteration but didn't do so completely -- in the first call, you're using pipefds[j-2] and in the next call you're using pipefds[j+1]. Whatever happened to j-1 on the first iteration? It is ignored. Is this intentional? j is only referenced on the next iteration (via the j+=2 .. [j-2]). Will anything ever reference the next-to-last entry in pipefds[]? Is that intentional too?

sarnold
  • 102,305
  • 22
  • 181
  • 238
  • Oh my god. Thanks. I didn't noticed that. Yeah I nested the braces poorly. Corrected them now. Secondly, yeah i did change `j+=2`from a `j++` from an earlier code. I've changed it back. Actually I use `j` to check only if the current command isn't the first command. If it isn't I dup else I don't. So this solved the problem of bad file descriptors. What do you mean by `Will anything ever reference the next-to-last entry in pipefds[]`? – mkab Dec 08 '11 at 10:10
  • Oh. Do you mean how do I check if it's the last command on the stack? Yeah I need to check that. It wasn't intentional. I didn't think about that... careless me. Will `if(j != 0 && j!= numPipes -1)` do? – mkab Dec 08 '11 at 10:15
  • For the `next-to-last entry in pipefds[]`, it was just noticing that you've got a "sliding window" of sorts, that works on `j-2`, `j+1`, and increments `j` by `2` (and skips the first `j==0`). That will **select** [ **0**, 1, **2**, **3**, **4**, **5**, 6, **7** ] when `numPipes==4`. (Cut holes in a sheet of paper for `j-2` and `j+1` and slide it along the text.) Perhaps that's intentional, perhaps not, but I'm running out of brainpower for determining. :) – sarnold Dec 08 '11 at 10:21
  • That's not intentional. Actually I'm not using `j+=2` anymore. I'm using `j++` now. When I used `j+2` i called `dup2(pipefds[(j-1) * 2])` and `dup2(pipefds[j * 2 + 1], 1)`. That was then. But now I'll just stick with `j++` and use `j-2`, `j+1`. Oh please don't run out of brainpower now :). I still need some help. After correcting everything, the pipe isn't working properly. When I type something like `ls | grep bin` no results is found. And when I want to type another command, it hangs. – mkab Dec 08 '11 at 10:27
  • Wait i wrote nonsense above. Currently now after changing codes, I increment with `j+=2` and call `(dup2(pipefds[j-2], 0) ` and `(dup2(pipefds[j+1], 1)`. – mkab Dec 08 '11 at 10:38
  • After various test the piping function doesn't still work. I tried adding a new condition to detect if it has reached the last command. Please check my edited question. – mkab Dec 08 '11 at 11:18
  • Found my error. My implementation of `wait` wasn't right. You have to put it in a for loop so that the parent waits for all of its the children. Thanks for your help though. Pointed me in the right direction. Cheers :) – mkab Dec 08 '11 at 23:44
  • Excellent! I think you should add that as an answer, so it'll be more obviously visible to others in the future. – sarnold Dec 09 '11 at 00:01
0

Here's the answer to the problem. Hopes it helps someone out there. I finally decided to increment j by 2 (j+=2). Function countPipes(char*) is just a simple function to count the number of pipes(|) from a char*

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

    int status;
    int i = 0;
    pid_t pid;

    int pipefds[2*numPipes];//declare pipes

    /**Set up pipes*/
    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
            if(j != 0 ){
                if(dup2(pipefds[j-2], 0) < 0){
                    perror(" dup2");///j-2 0 j+1 1
                    exit(EXIT_FAILURE);

                }
            }

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

            //execute commands
            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;//go to the next command in the linked list
        j+=2;//increment j
    }

    /**Parent closes the pipes and waits for all of its children*/

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

    for(i = 0; i < numPipes + 1; i++) //parent waits for all of its children
        wait(&status);
}
mkab
  • 933
  • 4
  • 16
  • 31