Shell Implementation not working correctly

	//save in/out
	int tmpin = dup(0);
	int tmpout = dup(1);
	
	//set initial input
	int fdin;
	if(_inputFile)
	{
	   fdin = open(_inputFile, O_RDONLY | O_CREAT, S_IREAD | S_IWRITE);
	}
	else
	{
	   //use default input
	   fdin = dup(tmpin);
	}

	int ret;
	int fdout;
	for(int i = 0; i < _numberOfSimpleCommands; i++)
	{
	   //redirect input
	   dup2(fdin, 0);
	   close(fdin);
	   //setup output
	   if(i == _numberOfSimpleCommands-1) 
	   {// last command must be set so output redirection is set

	      if(_outFile)
	      {
	         fdout = open(_outFile, O_WRONLY | O_CREAT, S_IREAD | S_IWRITE);
	      }
	      else
	      {  
	         fdout = dup(tmpout);
	      }
	   }
	   else
	   {
                   //simple command
	      //create pipe
	      int fdpipe[2];
	      pipe(fdpipe);
                  fdout = fdpipe[0];
                  fdin = fdpipe[1];
	    }

	    //redirect output
	    dup2(fdout, 1);
	    close(fdout);

	    //create child process
	    ret = fork();
	    if(ret == 0)
	    {
	       execvp((*_simpleCommands)._arguments[0],(*_simpleCommands)._arguments);
	       perror("execvp");
	       exit(1);
	     }//if ret
	}//for

	//restore in/out defaults
	dup2(tmpin, 0);
	dup2(tmpout, 1);
	close(tmpin);
	close(tmpout);
	
	if(!_background)
	{
		//wait for last command
		waitpid(ret, 0, 0);
	}

This is the code i have for the implementation of a shell that supports pipes and i/o redirection

So far this code has worked for one input and one outout redirection.
But will not work for pipeing, and i cannot for the life of me figure out why.
Ex. cat file1.cc | grep malloc

A simple command in my implementation is just a command with its arguments.

Any help would be great. Also please tell me if i need to clarify anything.

Just on code inspection....

I would close tmpin and tmpout in the child process.

Remember a child inherits every file descriptor, not just 0,1 and 2.

So the 2nd child will be holding the write end to it's own stdin open.

This is how i see my code working with the file descriptors and pipes and well every thing. Take a look at the picture to see what i am talking about, it is very descriptive and took a long time to do in paint so please take a look.

Do you understand what I mean here?

After the fork, in the child's leg, close these descriptors because the child does not want/need/expect them.

At the point when fork is called, the parent process is not pointing to the standard input and output, tmpin and tmpout. So wouldnt the child not have anything pointing to those after fork?

As far as I can see both tmpin and tmpout are both open immediately prior to the fork and hence will be both be open for both parent and child.

A suggestion, whenever you close a file descriptor, put -1 in the variable holding it. Then put in some debugging code immediately after the fork as follows...

.....
ret=fork();
fprintf(stderr,"pid=%d,ppid=%d,tmpin=%d,tmpout=%d\n",
getpid(),getppid(),tmpin,tmpout);
fflush(stderr);
if (ret==0)
...

See when i do something like this:
tmpin = dup(0);
I am just saving the fact that my parent process's 0th file descriptor was pointing to stdin in tmpin.

By the time my program gets to fork(), the 0th file descriptor is eather pointing to a new input file or stdin if there was not one. And in both cases the child will need were ever the 0th is pointing to. It may be tmpin or not but if it is it will need that open so it can use it.

Yes, tmpin will be 3, and tmpout will be 4. These will both be open file descriptors that the child processes will inherit.

Ok yes i see. I had a different idea of what dup did, i have only been doing this Unix stuff for about a week. But in reallity it wont really change my program to close those because they will just close when the child process closes, its just messy. The real question is why my stupid pipes are not working. It Makes No Sense! . But thanks man.

I'll give it a compile and have a look.

Oh, and another peeve of mine, don't use C++ comments in C code.

EDIT: Oops, this is actually C++ code as the for statement defines the variable.

man pipe

fdin=fdpipe[0];
fdout=fdpipe[1];

the first element is the read end, the second element is the write end of the pipe.

On some platforms(Solaris) pipes are bidirectional, but this is classical UNIX.

Also, you need to reap (with some form of wait) all the pids you get back from fork, not just the last one, else you end up with zombies, so I would expect you to capture all those pids.

Yes my good man. But if you look at the drawing and think about it the output of the command that is going to run will need to be written to the input of next command. So making the output write to that file will pipe over to the input file and the 0th descriptor will be set to that at the top of the loop. Just look at the pics and think about it and you will get it.

Perhaps, but the difference between you and me is I have a working pipeline from your code with that small change.

$ uname -a
SunOS foobar 5.7 Generic_106541-08 sun4c sparc SUNW,Sun_4_50
$ /usr/local/bin/gcc ab.c -o ab
$ echo "fred" | ./ab
fdout=6,fdin=5
pid=274,tmpin=3,tmpout=4
fdout=6,fdin=5
pid=275,tmpin=3,tmpout=4
0+1 records in
0+1 records out
0+1 records in
0+1 records out
fdout=6,fdin=5
pid=276,tmpin=3,tmpout=4
0+1 records in
0+1 records out
pid=277,tmpin=3,tmpout=4
fred
0+1 records in
0+1 records out

and the complete source should look very familiar....

#include <sys/types.h>
#include <sys/stat.h>
#include <sys/wait.h>
#include <stdio.h>
#include <unistd.h>
#include <errno.h>
#include <stdlib.h>
#include <fcntl.h>

struct cmd
{
  char **_arguments;
};

char *args[]={
	"dd",NULL,
	"dd",NULL,
	"dd",NULL,
	"dd",NULL
};

struct cmd cmdList[]={
	{args+0},
	{args+2},
	{args+4},
	{args+6}
};

struct cmd *_simpleCommands[]={
	cmdList+0,
	cmdList+1,
	cmdList+2,
	cmdList+3
};

char *_inputFile;
char *_outFile;

int main(int argc,char **argv)
{
int _background=0;
int _numberOfSimpleCommands=(sizeof(cmdList)
									/sizeof(cmdList[0]));
	int tmpin=dup(0);
	int tmpout=dup(1);
	int ret=-1;
	int fdout=-1;
	int fdin=-1;
	int i;
	int rc=0;

	if(_inputFile)
	{
	   fdin = open(_inputFile, O_RDONLY | O_CREAT, S_IREAD | S_IWRITE);
	}
	else
	{
	   fdin=dup(tmpin);
	}

	for (i = 0; i < _numberOfSimpleCommands; i++)
	{
	   rc=dup2(fdin, 0); if (rc==-1) perror("dup2(fdin)");
	   close(fdin); fdin=-1;

	   if(i == _numberOfSimpleCommands-1) 
	   {
	      if(_outFile)
	      {
	         fdout=open(_outFile, O_WRONLY | O_CREAT, S_IREAD | S_IWRITE);
	      }
	      else
	      {  
	         fdout=dup(tmpout);
	      }
	   }
	   else
	   {
	      int fdpipe[2];
	      pipe(fdpipe);
          fdin=fdpipe[0];
          fdout=fdpipe[1];

			fprintf(stderr,"fdout=%d,fdin=%d\n",fdout,fdin);
	    }

	    rc=dup2(fdout,1); if (rc==-1) perror("dup2(fdout)");

	    close(fdout); fdout=-1;

	    ret = fork();
	    if(ret==0)
	    {
			fprintf(stderr,"pid=%d,tmpin=%d,tmpout=%d\n",
					getpid(),tmpin,tmpout);			
			rc=close(tmpin); if (rc) perror("close(tmpin)");
			rc=close(tmpout); if (rc) perror("close(tmpout)");

			execvp( (*_simpleCommands)._arguments[0],
					(*_simpleCommands)._arguments);

	       	perror("execvp");
	       	exit(1);
	     }
	}

	rc=dup2(tmpin, 0); if (rc==-1) perror("dup2(tmpin,0)");
	rc=dup2(tmpout, 1); if (rc==-1) perror("dup2(tmpin,0)");
	close(tmpin);
	close(tmpout);
	
	if(!_background)
	{
		waitpid(ret, 0, 0);
	}

    return 0;
}

Hey Guys,

I was reading your thread because I have a similar project I am working on. I was unable to get either one of your code to work successfully. Do you mind elaborating or posting more of the code?

thanks,
Brian