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

This is a discussion on Code review for a class that spawns a new process and allows IPC within the C++ Programming forums, part of the General Programming Boards category; I have written a simple class that starts a process and allows the "host" process to do line-based communication with ...

  1. #1
    Registered User
    Join Date
    Dec 2006
    Location
    Canada
    Posts
    3,167

    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.
    Attached Files Attached Files
    Last edited by cyberfish; 05-23-2009 at 09:16 PM. Reason: updated attachment

  2. #2
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,310
    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.
    Last edited by laserlight; 05-23-2009 at 12:54 AM.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  3. #3
    Registered User
    Join Date
    Dec 2006
    Location
    Canada
    Posts
    3,167
    Oh =S thought I posted in C++ forum. Guess not.

    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 .

  4. #4
    Registered User
    Join Date
    Dec 2006
    Location
    Canada
    Posts
    3,167
    Fixed.

    Thanks!
    Attached Files Attached Files

  5. #5
    'Allo, 'Allo, Allo
    Join Date
    Apr 2008
    Posts
    611
    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

  6. #6
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,310
    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.
    Last edited by laserlight; 05-23-2009 at 12:05 PM.
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  7. #7
    Registered User
    Join Date
    Dec 2006
    Location
    Canada
    Posts
    3,167
    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.

    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.

    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.

    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.

    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!
    Attached Files Attached Files

  8. #8
    Registered User
    Join Date
    Dec 2006
    Location
    Canada
    Posts
    3,167
    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).

  9. #9
    C++ Witch laserlight's Avatar
    Join Date
    Oct 2003
    Location
    Singapore
    Posts
    21,310
    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");
    C + C++ Compiler: MinGW port of GCC
    Version Control System: Bazaar

    Look up a C++ Reference and learn How To Ask Questions The Smart Way

  10. #10
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    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.
    "I am probably the laziest programmer on the planet, a fact with which anyone who has ever seen my code will agree." - esbo, 11/15/2008

    "the internet is a scary place to be thats why i dont use it much." - billet, 03/17/2010

  11. #11
    Registered User
    Join Date
    Dec 2006
    Location
    Canada
    Posts
    3,167
    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.

    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.

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

    By the way, instead of writing assert(false) with a comment, write:
    Ah, didn't know that. Thanks.

    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).

    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!
    Attached Files Attached Files

Popular pages Recent additions subscribe to a feed

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21