Re: [boost] Boost.Process article: Tutorial and request for comments

Subject: Re: [boost] Boost.Process article: Tutorial and request for comments
From: Phil Endecott (spam_from_boost_dev_at_[hidden])
Date: 2009-04-21 14:15:11

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.