0

I'm writing a basic shell for course homework that will find a command in the given list of paths, and execute the command. It is also meant to handle pipes. However, when I fork a child process, I get a "Write error : Broken Pipe" message in gdb, and the program terminates abruptly.

I cannot seem to understand why this is happening, since I've been cautious about opening and closing correct pipes and process forking seems to work as desired. Can someone with more experience in C and unix programming please help me diagnose the problem? Is there something logically incorrect with my fork implementation / pipe implementation?

//commands is of the format {"ls -al", "more", NULL}
//it represents commands connected by pipes, ex. ls -al | more
char **commands = parseArgv(consoleinput, SPECIAL_CHARS[4]);

int numcommands = 0;

while( commands[numcommands]!=NULL )
{
    numcommands++;
}

const int  numpipes = 2*(numcommands-1);

int pipefds[numpipes];

int i=0;
for(i=0; i<numpipes;i=i+2)
{
    pipe(pipefds+i);
}

int pipe_w = 1;
int pipe_r = pipe_w - 3;
int curcommand = 0;

while(curcommand < numcommands)
{
    if(pipe_w < numpipes)
    {
        //open write end
        dup2(pipefds[pipe_w], 1);
    }

    if(pipe_r > 0)
    {
        //open read end
        dup2(pipefds[pipe_r], 0);
    }

    for(i=0;i<numpipes;i++) //close off all pipes
    {
        close(pipefds[i]);
    }

    //Parse current command and Arguments into format needed by execv

    char **argv = parseArgv(commands[curcommand], SPECIAL_CHARS[0]);

    //findpath() replaces argv[0], i.e. command name by its full path ex. ls by /bin/ls
    if(findPath(argv) == 0)
    {
        int child_pid = fork();

        //Program crashes after this point
        //Reason: /bin/ls: write error, broken pipe

        if(child_pid < 0)
        {
            perror("fork error:");
        }
        else if(child_pid == 0)     //fork success
        {
            if(execv(argv[0], argv) == -1)
            {
                perror("Bad command or filename:");
            }

        }
        else
        {
            int child_status;
            child_pid = waitpid(child_pid, &child_status, 0);
            if(child_pid < 0)
            {
                perror("waitpid error:");
            }
        }
    }
    else
    {
        printf("Bad command or filename");
    }
    free(argv);

    curcommand++;
    pipe_w = pipe_w + 2;
    pipe_r = pipe_r + 2;
}

//int i=0;
for(i=0;i<numpipes;i++) //close off all pipes
{
    close(pipefds[i]);
}

free(commands);
WhozCraig
  • 65,258
  • 11
  • 75
  • 141
user1897101
  • 1
  • 1
  • Can you construct a [minimal test-case](http://sscce.org)? – Oliver Charlesworth Feb 10 '13 at 17:14
  • You duplicate the pipe descriptors before you close the normal standard in/out descriptors, _always check for errors_. You also should duplicate them in the child. – Some programmer dude Feb 10 '13 at 17:17
  • Unrelated: `if(pipe_r > 0)` should be `if(pipe_r >= 0)` IMHO. – wildplasser Feb 10 '13 at 17:17
  • If I'm reading this right, you'll close all the pipefds on the first pass of the loop. That doesn't seem productive. – ipmcc Feb 10 '13 at 17:19
  • Thanks for the help everyone. Sorry about the typo, it is if(pipe_r >= 0)... pipe_w-3 since the the current command should read from the read end of the last command, which is two elements away. The error checking tip, (dup2(pipefds[pire_r]) < 0), was a big help to diagnose broken pipes. – user1897101 Feb 11 '13 at 20:46

1 Answers1

0

Duplicating the file descriptors after the fork() call, i.e. in the child process, is the correct way. Also, the waitpid() call makes one child process wait for the other, and the shell hangs. The wait() call should be moved to after the loop, i.e. the parent should wait for all the children.

user1897101
  • 1
  • 1