Code review for a class that spawns a new process and allows IPC

I have written a simple class that starts a process and allows the "host" process to do line-based communication with it. I would like a review of both the design and the implementation. Mostly the implementation, since this is the first time I've used most of the POSIX functions used here (pipe, read, write, dup2, etc). It appears to work, as far as I can test, but is there anything I am doing that's dangerous, insecure, may break easily, already broken... etc?

All advices welcomed!

Many thanks!

Header and cpp attached.

As an aside, is there a way to do this using threads? (as opposed to processes)

exec apparently won't work because it will replace the whole process.

I am not sure if this should be in the Linux programming forum instead, but...

*Moved to C++ Programming*

EDIT:
Why not pass std::string objects by const reference? It also looks like the start member function's implementation should be broken down into helper functions. One immediately obvious candidate is a function that returns the path + filename given the command. Another obvious candidate is a function that suitably formats the command for execv.

Oh =S thought I posted in C++ forum. Guess not.

Quote:

---

Why not pass std::string objects by const reference? It also looks like the start member function's implementation should be broken down into helper functions. One immediately obvious candidate is a function that returns the path + filename given the command. Another obvious candidate is a function that suitably formats the command for execv.

---

Thanks. Good idea, will do that :).

Fixed.

Thanks!

All of the deletes are using the wrong version and the s_args_ptr contents leak if fork fails. I'd also be inclined to make writeline take it's argument by value and push_back the \n before write-ing the c_str(), but that's stylistic rather than a bug. Still, less code gives you less chance to nause it up :)

Quote:

---

Originally Posted by adeyblue

All of the deletes are using the wrong version

---

Yes, and an appropriate fix for that is to use std::vector (nested, if necessary) instead of manual memory management.

Quote:

---

Originally Posted by adeyblue

I'd also be inclined to make writeline take it's argument by value and push_back the \n before write-ing the c_str(), but that's stylistic rather than a bug.

---

I would second this as an alternative to using std::vector for that case, though you do not necessarily need to change the interface since you can make an internal copy (but pass by value when you need to copy anyway can allow a compiler better optimisation opportunities). You can also do without the local variable named r since it does not really add any clarity to your code over the direct use of the return value of the write function call.

EDIT:
Consider placing your helper functions into an anonymous namespace in the source file instead of making them private member functions since they do not need to access the internals of the class.

Quote:

---

All of the deletes are using the wrong version and the s_args_ptr contents leak if fork fails. I'd also be inclined to make writeline take it's argument by value and push_back the \n before write-ing the c_str(), but that's stylistic rather than a bug. Still, less code gives you less chance to nause it up

---

Thanks! Good points. Haven't thought about passing the string by value and appending the \n, but I think it's a good idea, too.

Quote:

---

Yes, and an appropriate fix for that is to use std::vector (nested, if necessary) instead of manual memory management.

---

I didn't think that will work since the vector is to be directly passed to execv(), which takes an array of pointers.

Quote:

---

you do not necessarily need to change the interface since you can make an internal copy (but pass by value when you need to copy anyway can allow a compiler better optimisation opportunities)

---

I can still change the interface, so it's okay.

Quote:

---

You can also do without the local variable named r since it does not really add any clarity to your code over the direct use of the return value of the write function call.

---

Thanks, makes sense.

Quote:

---

Consider placing your helper functions into an anonymous namespace in the source file instead of making them private member functions since they do not need to access the internals of the class.

---

Good idea. Will do. I have never heard about anonymous namespaces before actually. Just looked it up.

BTW, how can I have separate prototypes and functions for functions in an anonymous namespace?

Fixed version attached.

Thanks for the great advices!

Another question. Should I declare my readline and writeline functions constant?

They don't modify anything in the class as far as C++ is concerned, but they do change the state of what the class abstracts (the child process).

Quote:

---

Originally Posted by cyberfish

I didn't think that will work since the vector is to be directly passed to execv(), which takes an array of pointers.

---

Ah yes, I see your problem. However, you could convert the vector of vectors of char into a vector of pointers to char that do not own what they point to (and add the null pointer terminator at that point), and then pass the latter to execv() after conversion to a pointer to pointer to char. If not, the technically correct thing to do to handle std::bad_alloc being thrown is to wrap the whole for loop in a try block, then catch the exception and delete[] those dynamic arrays that have already been created.

Quote:

---

Originally Posted by cyberfish

how can I have separate prototypes and functions for functions in an anonymous namespace?

---

Why would you need to have prototypes for your helper functions that are just implementation detail?

Quote:

---

Originally Posted by cyberfish

Should I declare my readline and writeline functions constant?

They don't modify anything in the class as far as C++ is concerned, but they do change the state of what the class abstracts (the child process).

---

hmm... if it only just happens that this particular implementation does not change the state of the member variables then perhaps leaving them non-const is better. get_state() is more obviously a candidate to be declared const.

By the way, instead of writing assert(false) with a comment, write:

Code:

---

assert(!"execv returned in the child process");

---

Since you don't use vectors in your header, you don't need to include it in the header.
What about returning an enum for your error codes instead of individual int values?
Documentation about what each function does, what the expected parameters & return values are would also be nice; I'd recommend Doxygen style comments.

Quote:

---

Ah yes, I see your problem. However, you could convert the vector of vectors of char into a vector of pointers to char that do not own what they point to (and add the null pointer terminator at that point), and then pass the latter to execv() after conversion to a pointer to pointer to char. If not, the technically correct thing to do to handle std::bad_alloc being thrown is to wrap the whole for loop in a try block, then catch the exception and delete[] those dynamic arrays that have already been created.

---

I see your point. I think I am going to keep it the way it is for now, though, since adding a second vector makes it a lot more complex (IMHO). I'd rather deal with dynamic allocations.

Quote:

---

Why would you need to have prototypes for your helper functions that are just implementation detail?

---

Just so I can see a list of (helper) functions.

Quote:

---

get_state() is more obviously a candidate to be declared const.

---

Done :).

Quote:

---

By the way, instead of writing assert(false) with a comment, write:

---

Ah, didn't know that. Thanks.

Quote:

---

Since you don't use vectors in your header, you don't need to include it in the header.

---

Ah, thanks. The header used to have a function that returns a vector (before I moved it to the anonymous namespace in the .cpp).

Quote:

---

What about returning an enum for your error codes instead of individual int values?

---

Good idea. Thanks.

I will look into Doxygen. My main goal for this little project is to learn to use a few tools - SVN, make, and Doxygen (haven't started on that).

New code attached.

Thanks!