Boris Schaeling wrote:
I've written an article about Boost.Process: http://www.highscore.de/cpp/process/
Hi Boris, I've quickly looked at your article; here is some feedback: First a general comment about how I would prefer that portable system API wrappers are implemented. No doubt other would have different views. Ideally, I'd like to see separate thin wrappers for Windows and POSIX that just "C++-ify" those APIs. Then on top of that implement a portability layer. My reason is this: I rarely if ever care about Windows and I already know how the POSIX APIs work (i.e. what the functions are called and what the semantics are). So please don't hide the bottom level thin wrappers that you must have as an implementation detail. Re the environment: I'm not enthusiastic that you have copied the environment into your own data structure. Why can't your environment type just wrap getenv() and setenv()? i.e. class environment { struct env_var_proxy { const char* varname; env_var_proxy(const char* varname_): varname(varname_) {} operator const char*() const { return getenv(varname); } void operator=(const char* newval) { setenv(varname,newval,1); } } public: env_var_proxy operator[](const char* varname) { return env_var_proxy(varname); } void erase(const char* varname) { unsetenv(varname); } }; Re starting processes: This example code: std::string exec = find_executable_in_path("notepad.exe"); std::vector<std::string> args = boost::assign::list_of("notepad.exe"); context ctx; ctx.environment = self::get_environment(); launch(exec, args, ctx); Is actually more verbose than directly calling the POSIX functions: int child_pid = fork(); if (child_pid == -1) throw "fork failed"; if (child_pid == 0) { execlp("notepad.exe","notepad.exe",NULL); _exit(1); } Why don't you boil it down to: launch_process("notepad.exe"); ? You write:
it is very important to refer to the executable with an absolute path - on all platforms including POSIX systems
Why do you say that? It's not clear to me if your terminate() calls wait() or not. Beware of processes that ignore SIGCHLD.
3) Currently Boost.Process is really only useful for creating child processes. It's not possible for example to detect and iterate over other processes on a system. I guess Boost.Process should be enhanced here. Then one day the well-known utility 'ps' on POSIX systems could be implemented in Boost C++. Any comments?
What is the use case for this?
4) When a process is created the standard streams (stdin, stdout and stderr) are closed by default. Is this a good choice?
No, IMHO.
If it isn't what should happen otherwise? Inherting standard streams (which is the default behavior with fork on POSIX systems)?
Yes.
5) Boost.Process requires to specify an absolute path to an executable which should be started. There is a helper function which returns the absolute path for an executable name (it uses the environment variable PATH). The question is if this should be done automatically if no absolute path is given?
exec*p() does that for you.
8) There is a method wait() which makes it possible to wait for another process to exit. Currently wait() is provided by the class child though. Shouldn't it really be defined by the class process (so you can wait not only for child processes to exit)?
Are you certain that wait() can be used to monitor non-child processes? I don't believe that it can.
9) As wait() blocks an asynchronous alternative should be provided.
Yes. When I've done this sort of thing I've used a SIGCHLD signal handler that pokes the main event loop; that loop calls a non-blocking wait(). It has to be non-blocking because you may get only one SIGCHLD for multiple children, so the event loop needs to keep calling wait() until there is nothing to wait for. Regards, Phil.