Thread: Pipe class.

  1. #1
    Registered User
    Join Date
    Oct 2007
    Posts
    242

    Pipe class.

    I've written this today, very easy to use:
    http://pastebin.com/m1c430b6a
    Code:
    /*				PIPES HANDLING CLASS
     * 			written by eXeCuTeR, 19.8.08
     * Works only on GNU/Linux or any UNIX system.
     * DESCRIPTION:
     * This class is used for pipes handling between a parent process and a child process, or between threads in a process,
     * it is very easy to use and maintain.
     * 
     * Creating a new instance of the class:
     * Pipe *inst = new Pipe(1024); 
     * 
     * Creating a new child:
     * inst->create_child("ls", my_argument_list);
     * (my_argument_list is an array of char pointers (char *args[]) which holds the arguments of the process if there's any.
     * note that the last node of the array must be NULL, e.g: char *my_args[] = { "ps", "-e", NULL }; otherwise it will fail.
     * if there's no arguments, just pass NULL.
     * 
     * Writing and reading data from the pipe:
     * inst->Write("Hello world!");
     * cout << inst->Read();
     * 
     * NOTE: the child's stdin is the reading stream, and his stdout is the writing stream (i.e, when you send data from the parent process using Write(),
     * it will be also put in the child's stdin. Also, when you printf/cout (or sending data to the stdout) in the child process, you are actually sending data to the pipe
     * which can be read in the parent process.
     * 
     * Happy pipping!
    */
    
    #include <iostream>
    #include <cstdlib>
    #include <signal.h>
    #include <sys/types.h>
    #include <sys/wait.h>
    #include <unistd.h>
    #include <cstring>
    #include <cassert>
    
    #define CRLF "\r\n"
    #define NEWLINE "\n"
    
    using namespace std;
    
    class Pipe
    {
    	private:
    		int *pipe_fds, buffer_size, temp_size;
    		char *data, *temp_buffer, *temp_data;
    		enum { READING, WRITING };
    		
    		int ReadyToRead(); // specifies CRLFs in the end of the stream
    		int cut_crlf(char *str); // cut CRLFs from a string
    	public:
    		Pipe(int size); // creates the pipe with a specific size of buffer
    		void create_child(const char *cmd, const char **arguments); // creates a child process
    		char *Read(); // reads data from the stream (returns the data that was read from the pipe, or NULL on failure)
    		int Write(const char *data); // sends data to the stream (returns the numbers of bytes that were written, or -1 on failure)
    		inline int get_pipe_r() { return this->pipe_fds[READING]; } // returns the reading pipe
    		inline int get_pipe_w() { return this->pipe_fds[WRITING]; } // returns the writing pipe
    		~Pipe(); // frees resources
    };
    
    int main(void)
    {
    	Pipe *a = new Pipe(11);
    	a->create_child("child process", NULL);
    	/*
    	 *    the child process contains:
    				char buffer[256];
    				fgets(buffer, sizeof(buffer), stdin); // this will be read from the pipe
    				fputs("Hey, how you doing?\n", stdout); // this will be sent to the pipe
    	*/
    	a->Write("Hello world"); // this will be entered to buffer because fgets is blocking until some data is in the stdin stream
    	a->Write("Hey, thanks!"); // this will be entered to the stream
    	char *data = a->Read(); // the data is received from the pipe
    	cout << data;
    	return 0;
    }
    
    Pipe::Pipe(int size)
    {
    	pipe_fds			= new int[2];
    	pipe(this->pipe_fds);
    	
    	assert(size > 10);
    	
    	this->buffer_size 	= size;
    	this->data 			= new char[this->buffer_size];
    	this->temp_buffer	= new char[this->buffer_size];
    	this->temp_data		= new char[this->buffer_size];
    	this->temp_size 	= this->buffer_size;
    	
    	memset(this->temp_buffer, '\0', this->buffer_size);
    	memset(this->data, '\0', this->buffer_size);
    	memset(this->temp_data, '\0', this->buffer_size);
    }
    
    void Pipe::create_child(const char *cmd, const char **arguments)
    {
    	pid_t fork_ret = fork(); // creating a new child
    	if(fork_ret == (pid_t)0) // this is the child
    	{
    		assert(cmd != NULL);
    		dup2(this->pipe_fds[READING], STDIN_FILENO); // the child's stdin stream is now redirected to the reading stream
    		dup2(this->pipe_fds[WRITING], STDOUT_FILENO);	// the child's stdout stream is now redirected to the writing stream
    		execv(cmd, (char * const *)arguments); // executing the child
    		cerr << "An error occurred - could not run child process.\n"; // in case execv failed
    		exit(1);
    	}
    }
    
    char *Pipe::Read()
    {
    	if(this->ReadyToRead() == 0)
    	{
    		cerr << "Couldn't read from the stream.\n";
    		return NULL;
    	}
    	else
    	{
    		/* Reading from the stream */
    		unsigned int bytes_read = 0;
    		int temp = 0;
    		char *bytes = new char[this->buffer_size];
    		memset(bytes, '\0', this->buffer_size);
    		do
    		{
    			temp = read(this->pipe_fds[READING], this->temp_buffer, this->buffer_size);
    			if(temp == -1)
    				return NULL; // error
    			bytes_read += temp;
    			if(bytes_read > (unsigned int)this->temp_size)
    			{	
    				this->temp_size += bytes_read;
    				bytes = (char *)realloc(bytes, this->temp_size); // allocating more space for the rest of the data
    			}
    			if(strstr(this->temp_buffer, CRLF) != NULL)
    			{
    				strcat(bytes, this->temp_buffer);
    				break;
    			}
    			strcat(bytes, this->temp_buffer); // adding the new buffer to the data
    		} while(strstr(this->temp_buffer, CRLF) == NULL);
    		
    		bytes_read = this->cut_crlf(bytes);
    		if( (bytes_read - this->buffer_size) > 0)
    		{
    			this->temp_data = (char *)realloc(this->temp_data, (strlen(bytes) - sizeof(this->temp_data)));
    			memset(this->temp_data, '\0', sizeof(this->temp_data));
    		}
    		
    		strcpy(this->temp_data, bytes);
    		return this->temp_data;
    	}
    }
    
    int Pipe::Write(const char *data)
    {
    	/* Writing to the stream */
    	char *buffer = new char[strlen(data) + strlen(NEWLINE)];
    	strcat(buffer, data);
    	strcat(buffer, NEWLINE);
    	unsigned int bytes_written = 0;
    	int temp = 0;
    	do
    	{
    		temp = write(this->pipe_fds[WRITING], buffer + bytes_written, strlen(buffer + bytes_written));
    		if(temp == -1)
    			return -1;
    		bytes_written += temp;
    	} while(bytes_written < (strlen(data) + strlen(NEWLINE)));
    	return bytes_written;
    }
    
    int Pipe::cut_crlf(char *str)
    {
    	unsigned int i, k = 0;
    	for(i = 0; i < strlen(str) - 1; i++)
    	{
    		if(str[i] == '\r' && str[i + 1] == '\n')
    			memset(str + i, '\b', 2);
    		else
    			k++;
    	}
    	return k;
    }
    
    int Pipe::ReadyToRead() 
    { 
    	if(write(this->pipe_fds[WRITING], "\r\n\r\n\r\n\r\n\r\n\r\n", strlen("\r\n\r\n\r\n\r\n\r\n\r\n")) == -1)
    		return 0;
    	return 1; 
    }
    
    Pipe::~Pipe()
    {
    	close(this->pipe_fds[READING]);
    	close(this->pipe_fds[WRITING]);
    	delete this->pipe_fds;
    	delete this->temp_buffer;
    	delete this->data;
    }

  2. #2
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Code:
            int *pipe_fds
            ... 
    	pipe_fds			= new int[2];
    Surely this doesn't need to be a pointer:
    1. There is no variation in the number of file descriptors needed.
    2. You allocate the memory in constructor and free it in destructor.

    Code:
    char *bytes = new char[this->buffer_size];
    ...
    bytes = (char *)realloc(bytes, this->temp_size);
    Argh! Recipe for crash - do not mix C and C++ memory allocation - they are not all the same thing [they may be on some systems, but there is NO guarantee that for exampel new doesn't add some extra info on top of what malloc maintains, so when you use realloc, the data stored next to the data block doesn't contain what realloc expects].

    If this is a Linux/Unix system, then I expect linefeed is sufficient as line-ending. Most Linux systems do not issue CR + LF.


    Code:
    this->temp_data = (char *)realloc(this->temp_data, (strlen(bytes) - sizeof(this->temp_data)));
    What is that supposed to be? I don't think you really want to store bytes - sizeof(char *), really... Suprised it doesn't crash.

    Code:
    		return this->temp_data;
    If you return the buffer, how do you prevent this from going absolutely crazy if someone does:
    Code:
    char *msg1;
    char *msg2;
    Pipe pipe(1024);
    ...
    msg1 = pipe.Read();
    msg2 = pipe.Read();
    printf("Message 1: %s, Message 2: %s\n", msg1, msg2);

    It would make much more sense to pass in a buffer to fill in.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  3. #3
    Registered User
    Join Date
    Oct 2007
    Posts
    242
    Hey, thanks for the comments!

    About the sizeof....I didn't notice I used sizeof instead of strlen, that was totally dumb of me (guessing I was too tired or whatever)
    ---
    I need to use realloc in order to receive all the data in one chunk and not if my buffer_size is 10 and there's more than 10 bytes in the stream so the other bytes in the stream aren't read or read in the next time or you call Read().
    So I should use malloc in this case instead of new?
    ---
    Code:
    char *msg1;
    char *msg2;
    Pipe pipe(1024);
    ...
    msg1 = pipe.Read();
    msg2 = pipe.Read();
    printf("Message 1: &#37;s, Message 2: %s\n", msg1, msg2);
    That would work just fine.

  4. #4
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    I wouldn't use malloc in a C++ project. Just write your own "extend buffer" code, e.g.
    (pseudo-C++)
    Code:
        if (size > current_size)
        {
             size_t new_size current_size + something;
             char *temp = new char[new_size];
             memcpy(temp, buffer, current_size);
             delete buffer;
             buffer = temp;
             curent_size = new_size; 
        }
    Have you actually tried those two reads in a row - as far as I can tell, you are returning the same buffer each time, so both pointers would point to the same area of memory [unless you re-allocated]. I could be wrong - your code wasn't that easy to follow with several different buffers floating about.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  5. #5
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,895
    Just write your own "extend buffer" code, e.g.
    Whenevery you do something like that, first think if a vector wouldn't be more appropriate. You can use it with C APIs - &v[0] yields the address of the first element, and all elements are sequential.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  6. #6
    Kernel hacker
    Join Date
    Jul 2007
    Location
    Farncombe, Surrey, England
    Posts
    15,677
    Quote Originally Posted by CornedBee View Post
    Whenevery you do something like that, first think if a vector wouldn't be more appropriate. You can use it with C APIs - &v[0] yields the address of the first element, and all elements are sequential.
    Good point. In this case, a std::string may actually be even more suitable, since the result of the function is a C style string - turning that to a C++ std::string is probably not a bad thing.

    --
    Mats
    Compilers can produce warnings - make the compiler programmers happy: Use them!
    Please don't PM me for help - and no, I don't do help over instant messengers.

  7. #7
    Registered User
    Join Date
    Oct 2007
    Posts
    242
    Quote Originally Posted by matsp View Post
    I wouldn't use malloc in a C++ project. Just write your own "extend buffer" code, e.g.
    (pseudo-C++)
    Code:
        if (size > current_size)
        {
             size_t new_size current_size + something;
             char *temp = new char[new_size];
             memcpy(temp, buffer, current_size);
             delete buffer;
             buffer = temp;
             curent_size = new_size; 
        }
    Have you actually tried those two reads in a row - as far as I can tell, you are returning the same buffer each time, so both pointers would point to the same area of memory [unless you re-allocated]. I could be wrong - your code wasn't that easy to follow with several different buffers floating about.

    --
    Mats
    Well what I actually tried to do in Read() is to read all the data in one single chunk.
    For example, is the buffer_size is 10 bytes long, and the data in the pipe is more than 10 bytes (let's assume 15), it should also read the other 5 bytes that are left in the pipe until it reaches a CRLF (my identifier to an end of a stream of data - I added it so I can recognize when I should stop reading because I wanted to read in only one chunk)

    Anyways, thanks, I'll try that.

  8. #8
    Officially An Architect brewbuck's Avatar
    Join Date
    Mar 2007
    Location
    Portland, OR
    Posts
    7,396
    Many, many problems. Just starting at the beginning and going down...

    1. Why the requirement that the buffer size be greater than 10? Seems arbitrary.

    2. In create_child(), what if fork() fails?

    3. Nothing prevents somebody from calling create_child() multiple times. If they do so, the original pipes (as well as processes) are leaked.

    4. Read() is insanely complicated and buggy. You allocate char *bytes but never delete it. You mix C and C++ memory management. You treat a -1 return from read() as if it were an error -- if errno == EINTR then this is NOT an error, but you never check. You are using strcat() all over the place when it is definitely not necessary. You use strstr() to check for CRLF when this is unnecessary -- you already checked in the previous iteration, so any CRLF which is present could only possibly be at the end of the string but you uselessly check the entire thing. The whole function makes my head hurt.

    5. In Write, you once again treat a -1 return from write() as if it was an error, without checking if errno == EINTR. You redundantly call strlen() over and over on data which cannot change between calls.

    6. ReadyToRead() is simply inexplicable. Writing stuff to the pipe in order to read from it? What's going on?

    7. The process you forked has to be reaped, and yet you call wait() nowhere. A long-running program using this class will eventually fill the system with zombies.

    And of course, overarching design oddities. The class is called Pipe and yet it spawns processes. This thing isn't a Pipe, it's more like a ProcessWithCommunicationChannels that just happens to use pipes internally. It forces a particular protocol -- no '\0' bytes can be passed, and all communication is forced to use CRLF as message terminators. The use of CRLF itself is bizarre on a system which uses plain old LF as its line terminator. The naming convention is inconsistent, with some stuff being_written_like_this and other stuff BeingWrittenLikeThis.
    Last edited by brewbuck; 08-20-2008 at 08:40 PM.
    Code:
    //try
    //{
    	if (a) do { f( b); } while(1);
    	else   do { f(!b); } while(1);
    //}

  9. #9
    Registered User
    Join Date
    Oct 2007
    Posts
    242
    Quote Originally Posted by brewbuck View Post
    Many, many problems. Just starting at the beginning and going down...

    1. Why the requirement that the buffer size be greater than 10? Seems arbitrary.

    2. In create_child(), what if fork() fails?

    3. Nothing prevents somebody from calling create_child() multiple times. If they do so, the original pipes (as well as processes) are leaked.

    4. Read() is insanely complicated and buggy. You allocate char *bytes but never delete it. You mix C and C++ memory management. You treat a -1 return from read() as if it were an error -- if errno == EINTR then this is NOT an error, but you never check. You are using strcat() all over the place when it is definitely not necessary. You use strstr() to check for CRLF when this is unnecessary -- you already checked in the previous iteration, so any CRLF which is present could only possibly be at the end of the string but you uselessly check the entire thing. The whole function makes my head hurt.

    5. In Write, you once again treat a -1 return from write() as if it was an error, without checking if errno == EINTR. You redundantly call strlen() over and over on data which cannot change between calls.

    6. ReadyToRead() is simply inexplicable. Writing stuff to the pipe in order to read from it? What's going on?

    7. The process you forked has to be reaped, and yet you call wait() nowhere. A long-running program using this class will eventually fill the system with zombies.

    And of course, overarching design oddities. The class is called Pipe and yet it spawns processes. This thing isn't a Pipe, it's more like a ProcessWithCommunicationChannels that just happens to use pipes internally. It forces a particular protocol -- no '\0' bytes can be passed, and all communication is forced to use CRLF as message terminators. The use of CRLF itself is bizarre on a system which uses plain old LF as its line terminator. The naming convention is inconsistent, with some stuff being_written_like_this and other stuff BeingWrittenLikeThis.
    1. Yeah, I should erase that - that was used for bugs detecting, and I forgot to remove it.
    2. I will make a condition if it fails.
    3. How come will the pipes be leaked? Although I haven't thought of creating more then one child, but if I'll accept that and not change anything with it - I should make some more changes with Read() and Write() also so they also accept another parameter which is an ID (which I assign to it) of the created process so the user passes it with the Write() and Read() conventions so the functions could know what pipe to read/write to.
    In a second thought, I should create a linked list of the created children which will hold the reading & writing pipe, and the identifier for each process created, and every time a child is created, I'll add a new node to the list and return the ID of the process.
    This way, the user passes the ID, then I search for the write node (just a function that runs while a node * which points to the head of the list doesn't equal to null and searches in each iteration for that identifier. When found, I return the current node.
    4. I know that..that's because I tried to read all the data in ONE chunk as I explained previously. I'm gonna try matsp's version to realloc().
    5. Well, I don't think it's actually necessary, is it?
    6. ReadyToRead is used so I can figure out when the pipe ends and I've finished to read the user messages because apparently, while(!feof(fdopen(THE_PIPE, "r")) doesn't work and puts me in an infinite loop.
    I know it's not smart and stupid and may be really buggy, but I couldn't think of another way to read all the data and return it.
    7. Oh, forgot to add it here - I have created a struct sigaction and a signal handler for SIGCHLDs. The handler just wait()s, but I forgot to add it here.

    Thanks for the comments fellas, this is really helpful.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Class design problem
    By h3ro in forum C++ Programming
    Replies: 10
    Last Post: 12-19-2008, 09:10 AM
  2. Specializing class
    By Elysia in forum C++ Programming
    Replies: 6
    Last Post: 09-28-2008, 04:30 AM
  3. Defining derivated class problem
    By mikahell in forum C++ Programming
    Replies: 9
    Last Post: 08-22-2007, 02:46 PM
  4. matrix class
    By shuo in forum C++ Programming
    Replies: 2
    Last Post: 07-13-2007, 01:03 AM