UNIX Socket FAQ

A forum for questions and answers about network programming on Linux and all other Unix-like systems

You are not logged in.

  • Index
  • » Processes
  • » Problem while execv for a program using PIPES

#1 2011-01-18 07:07 PM

Pranay
Guest

Problem while execv for a program using PIPES

Hi all,
I am making a program for my class assignment related to PIPES.
In the project i need to implement a remote shell server which can take 'piping' commands form the user.
eg.
ls | cat    => as expected this will take output from ls into cat.
The problem is that i did all the piping etc. But when i finally run the code and 'ls' is executed first and it pipes its data into 'PIPE' (fd=1) and in the next iteration, the forked process which executes CAT also has its stdin as PIPE's fd=0(i used dup2 to do copy the file descriptors).

Now when finally the command is executed, CAT sends back the output as desired to the client but doesnot return for some reason(it just sleeps after sending the last line of output). The task manager in Ubuntu shows that its sleeping.
Because I use wait() for the child process to exit...so my program gets stuck there.
If I kill it from the task manager then the process returns to accept the next command but this problem has other effects which are not letting me finish my F##$#ing assigment.
I am really REALLY stuck and all my work will go down the drain....your help will be very much appreciated.

Where could I have been wrong :|

Thanks.
Pranay

Below is the main function I am using(hope its not that messy )
the struct command *comm -> this has the commands like LS and CAT stored in an array format.
sub_comm_count = number of commands in a single like e.g ls | cat =>2 commands so its value= 1
current_count = -1(at the first call)

void recursive_call(struct command *comm, int sub_comm_count,int current_count)
 {
     
     current_count++;   // if current_count==sub_comm_count then no more commands to process
     int childpid;
     if( (childpid=fork()) <0)
        errlog("fork error");
     else if (childpid==0)
     {
         /***********************
          *child process - Recursive_call()
          **********************/
         int flag[4];
         flag[0]=flag[1]=flag[2]=flag[3]=0;
          int read_pipe,fd_prog_to_file,fd_file_to_prog;
          read_pipe=read_from_pipe(comm_count);// checks and copies the pipe fd to read pending data from
          if(read_pipe==1)  flag[0]=1;
          if(read_pipe==-1)
          {
              //No PENDING pipe to read from
              /** check for '<' **/
              if (comm[current_count].file_to_program!=NULL)
              {
                if((fd_file_to_prog=open(comm[current_count].file_to_program,O_RDWR))==-1)
                    {
                        errlog("error opening 'read' file  \n");
                        exit(0);
                    }
                    flag[1]=1;
                // copy the file descriptor to the STDIN_FILENO
                if(dup2(fd_file_to_prog,STDIN_FILENO)!=STDIN_FILENO)
                {
                    errlog("error in dup2\n");
                    exit(0);
                }
              }
          }
          if(comm[current_count].pipe_level<0)
          {
              /** check for '>' **/
              if (comm[current_count].program_to_file!=NULL)
              {
                if((fd_prog_to_file=open(comm[current_count].program_to_file,O_WRONLY|O_CREAT, 0644))==-1)
                    {
                        errlog("error opening 'write' file  \n");
                        exit(0);
                    }
                    flag[2]=1;
                // copy the file descriptor to the STDOUT_FILENO
                if(dup2(fd_prog_to_file,STDOUT_FILENO)!=STDOUT_FILENO)
                {
                    errlog("cannot dup2\n");
                    exit(-1);
                }
              }
              else
                {
                    /** OUTPUT TO client SOCKET ==default output **/
                    ///CHANGE HERE SOCKET VALUE ---enter it from the funciton
                    if( dup2(newsockfd,STDOUT_FILENO)!= STDOUT_FILENO )
                     {
                        errlog("dup 2 error");
                        exit(-1);
                     }

                }

          }else{
                  close(pipe_from[comm[current_count].pipe_level + comm_count ][0]);
                 /** handle '|' --> STDOUT DATA piped into some PIPE **/
                 if( dup2(pipe_from[comm[current_count].pipe_level + comm_count ][1],STDOUT_FILENO)!= STDOUT_FILENO )
                 {
                    errlog("dup 2 error pipe out\n");
                    exit(-1);
                 }
                 flag[3]=1;

              }
         /** execute the command **/
          
             execv(comm[current_count].argv1[0],comm[current_count].argv1);
             errlog("execv did not work \n");
             exit(-1 );
         
     }
            /***********************
             *Parent process - Recursive_call()
             **********************/
             int dummy;
             wait(&dummy);
             if(sub_comm_count>current_count)
             {

                 comm_count++;
                 recursive_call(comm,sub_comm_count,current_count);
             }
             else
             {
                return ;
             }
 }

#2 2011-01-18 07:08 PM

Pranay Sharma
Guest

Re: Problem while execv for a program using PIPES

Well one more thing it also takes care of commands like
ls > output.txt
or cat < input.txt

but these are not a problem right now

#3 2011-01-18 08:17 PM

Pranay
Guest

Re: Problem while execv for a program using PIPES

Hi All,
Thanks but my problem has been solved now.
I was not able to get the EOF from the reading process as i had not closed all the write ends of my pipes.

Thanks anyways..
BR
PRanay

#4 2011-01-20 04:32 AM

i3839
Oddministrator
From: Amsterdam
Registered: 2003-06-07
Posts: 2,201

Re: Problem while execv for a program using PIPES

Hi Pranay, glad you solved your problem on your own, hope it's working fine now.

I'm going to give you some feedback on your code, hope you don't mind. Do with
it what you want, but I hope you'll use it to write better code in the future. The
below pretty much applies to everyone new to programming, so this is not just
for you, but also others reading this later.

First, your code is a bit messy, but not too bad, it could have been much worse.

It seems you're manually typing all spaces for indentation. I don't know what
text editor you use, but if you use one suitable for programming then it can do
most indentation automatically for you. Personally I use tabs instead of spaces,
but if you want spaces you can configure the editor to write 4 or 8 spaces every
time you hit TAB. If you find 8 space tabs too wide, you can configure it to show
tabs as 4 characters wide while still using hard tabs instead of spaces. For lining
things out I sometimes use spaces.

The main rule for coding style is that it's consistent. So if there is no coding
style given, use whatever you like, but be consistent. Example:

if( (childpid=fork()) <0)

Sometimes you have spaces between if and the (, sometimes you don't. Same for <, >
and == tests. The common style is:

if ((childpid = fork()) < 0)

Other common style thing is to use not more than one empty line between statements,
and to keep lines shorter than 80 characters. Historically in C, variable declarations
came before assignments and you weren't allowed to mix them. It still is good practice
to declare all variables you're going to use at the start of each block. So swap the
"int childpid;" and the "current_count++;". In C /* */ is usually used for comments,
and // is usually used to comment things out temporarily, or for notes that should be
removed in the future.

Assignments within if/while tests are best avoided when possible, so I'd write the above as:

childpid = fork();
if (childpid < 0)

This way one thing at a time happens and the code is clearer. Try to do only one
thing on each line.

In general, it's best to avoid recursion. This because recursion is tricky and hard to
think about. Only use recursion when the recursive version of the code is clearer
than the loop version. Any recursion can be rewritten into a loop form. Recursion
is good for things like traversing trees, where keeping track of all the intermediate
state is easier done on the stack than in some other way.

Now about naming things. Your variables have good names, they are to the point
and have meaning. However, "recursive_call" does not. It doesn't describe what
the function does, or what its purpose is. "flag" is a bit unclear too, but you're
not using it for anything as far as I can tell.

Another thing, recursive_call() is big and complex enough to be unwieldy. As it does
multiple quite different things, it's easy to split up into multiple smaller and simpler
functions. When things become complicated and big, it's usually time to look what
helper function to introduce, or how to split things up. I find that just writing code
and splitting it when it becomes too big works better than starting with many small
functions: You never know beforehand what a good interface is and what exactly
is needed, so I prefer the top down method.

The hardest part of programming is writing clear, simple code. When I program,
I continuously look for ways to simplify and improve my code. Clear code is easier
to verify, easier to debug and generally more pleasant to work with. All extra time
spend making the code clearer is recouped with less time spend on debugging.

Here's your code, but rewritten with the above comments in mind. I tried to keep
the behaviour the same. I'm sure it can be simplified a lot more, but to see how
I have to see the surrounding code too. Having too many levels of indentations
is usually a sign things can be simplified a lot. What I'm trying to show is that by
looking at your code you can find ways to simplify it without too much effort.
This is only one way to simplify it, the beauty (and hard part) of programming
is that there are near infinite number of ways to program the same thing.

static void file_to_stdin(const char *file)
{
	int fd = open(file, O_RDONLY);
	if (fd == -1){
		errlog("error opening 'read' file  \n");
		exit(1);
	}
	/* copy the file descriptor to the STDIN_FILENO */
	if (dup2(fd, STDIN_FILENO) == -1){
		errlog("error in dup2\n");
		exit(1);
	}
	close(fd);
}

static void file_to_stdout(const char *file)
{
	int fd = open(file, O_WRONLY|O_CREAT, 0644)
	if (fd == -1){
		errlog("error opening 'write' file  \n");
		exit(1);
	}
	/* copy the file descriptor to the STDOUT_FILENO */
	if (dup2(fd, STDOUT_FILENO) == -1){
		errlog("cannot dup2\n");
		exit(1);
	}
	close(fd);
}

static void exec_child(struct command *comm, int comm_count)
{
	int read_pipe;
	int flag[4] = {0};
	int fd = -1;

	/* checks and copies the pipe fd to read pending data from */
	read_pipe = read_from_pipe(comm_count);
	if (read_pipe == 1)
		flag[0] = 1;
	else if (read_pipe == -1){
		/* No PENDING pipe to read from. Check for '<' */
		if (comm->file_to_program){
			file_to_stdin(comm->file_to_program);
			flag[1] = 1;
		}
	}
	if (comm->pipe_level < 0){
		/* check for '>' */
		if (comm->program_to_file){
			file_to_stdout(comm->program_to_file);
			flag[2] = 1;
		} else {
			/** OUTPUT TO client SOCKET == default output **/
			// CHANGE HERE SOCKET VALUE ---enter it from the funciton
			if (dup2(newsockfd, STDOUT_FILENO) == -1){
				errlog("dup 2 error");
				exit(-1);
			}
			close(newsockfd);
		}
	} else {
		// This looks a bit dodgy, not sure what's going on here.
		int *from_pipe;
		
		from_pipe = pipe_from[comm->pipe_level + comm_count];
		/*
		 * handle '|' --> STDOUT DATA piped into some PIPE
		 */
		if (dup2(from_pipe[1], STDOUT_FILENO) == -1){
			errlog("dup 2 error pipe out\n");
			exit(-1);
		}
		close(from_pipe[0]);
		close(from_pipe[1]);
		flag[3] = 1;
	}
	/* execute the command */	
	execv(comm->argv1[0], comm->argv1);
	errlog("execv did not work \n");
	exit(-1);
}

void exec_all_cmds(struct command *comm, int sub_comm_count)
{
	int childpid;
	int current;
	int dummy;

	for (current = 0; current < sub_comm_count; count++){
		childpid = fork();
		if (childpid < 0){
			errlog("fork error");
			exit(1);
		} else if (childpid == 0){
			return exec_child(comm[current], current);
		}
		/*
		 * Parent process
		 */
		wait(&dummy);
	}
}

As you can see, with the rewritten code it's a lot easier to follow the logic,
because unrelated things are out of the way.

Offline

  • Index
  • » Processes
  • » Problem while execv for a program using PIPES

Board footer

Powered by FluxBB