Problem with piping commands in C

Question

I'm trying to create a simple shell in C for Unix. I've been able to do all the parsing of commands and execution, but I'm having a problem with piping. I think the problem is that I'm not hooking into the correct pipe for the input of the second command.

For example, if I type "ls | wc", it will pause after the "wc" command, which I think is because its waiting for input. I think the problem is when I use dup2(reading[i],0), and its not hooking into the correct pipe.

I know this is a bit of a broad question, but if there are any pointers I could get, I would appreciate it. Here is the code that creates new processes and tries to pipe them.

    int fileds[2];
    int reading[num_cmds];
    int writing[num_cmds];

    int p;
    for(p=0; p < num_cmds; p++)
    {
        reading[p] = -1;
        writing[p] = -1;
    }

    int j;
    for(j=0; j < num_cmds-1; j++)    //Create pipes for commands
    {
        int fileds[2];
        pipe(fileds);
        reading[j+1] = fileds[0];
        writing[j] = fileds[1];
    }

    int i = 0;
    for(i = 0; i < num_cmds;i++)
    {           
        cmd_args = parse_cmd(cmds[i],output_file,input_file,&run_bg); //Get command and args

        pid_t childpid;
        int status;
        childpid=fork();

        if (childpid >= 0) 
        {
            if (childpid == 0) 
            {               
                if(writing[i] != -1)
                {
                    dup2(writing[i],1);
                    close(writing[i]);
                }

                if(reading[i] != -1)
                {
                    dup2(reading[i],0);
                    close(reading[i]);
                }

                int h;
                for(h = 0; h < num_cmds; h++)
                {
                    close(writing[h]);
                    close(reading[h]);
                }

                if(execvp(cmd_args[0],cmd_args) == -1) 
                {
                    perror("Problem with command");
                    exit(0);
                }
            }
            else 
            {
                wait(&status);
                int m;
                for(m = 0; m < num_cmds; m++)
                {
                    if( writing[m] != -1) close(writing[m]);
                    if( reading[m] != -1) close(reading[m]);
                }
            }
        }
        else 
        {
             perror("fork"); 
             continue;
        }


        input_file[0] = 0;
        output_file[0] = 0;
        run_bg = 0;
    }

}

UPDATE: I was able to figure it out, thanks to Richard. It was a combination of closing the file descriptors in the wrong order and not closing some at all. Here's the working code.

int fileds[2];
    int reading[num_cmds];
    int writing[num_cmds];

    int p;
    for(p=0; p < num_cmds; p++)
    {
        reading[p] = -1;
        writing[p] = -1;
    }

    int j;
    for(j=0; j < num_cmds-1; j++)
    {
        int fileds[2];
        pipe(fileds);
        reading[j+1] = fileds[0];
        writing[j] = fileds[1];
    }

    int i = 0;
    for(i = 0; i < num_cmds;i++)
    {           
        cmd_args = parse_cmd(cmds[i],output_file,input_file,&run_bg);

        pid_t childpid;
        int status;
        childpid=fork();

        if (childpid >= 0) 
        {
            if (childpid == 0) 
            {               
                if(writing[i] != -1)
                {
                    close(1);
                    dup2(writing[i],1);
                }

                if(reading[i] != -1)
                {
                    close(0);
                    dup2(reading[i],0);
                }

                if(execvp(cmd_args[0],cmd_args) == -1) 
                {
                    perror("Problem with command");
                    exit(0);
                }
            }
            else 
            {

                wait(&status);
                close(writing[i]);

                if(i > 0) 
                {
                    close(reading[i]);
                }
            }
        }
        else 
        {
             perror("fork");
        }


        input_file[0] = 0;
        output_file[0] = 0;
        run_bg = 0;
    }

Answer 1

I think your problem may be that you wait for each process inside the loop and then close all the file descriptors. This makes the file descriptors invalid for the next call to dup2() and results in stdin for the next process staying unchanged.

Just a guess, I haven't run the code.