Re: [boost] [gsoc] Boost.Process done

Subject: Re: [boost] [gsoc] Boost.Process done
From: Boris Schaeling (boris_at_[hidden])
Date: 2010-09-10 18:21:48

On Fri, 10 Sep 2010 01:33:53 +0200, Ilya Sokolov <ilyasokol_at_[hidden]>  
wrote:

> [...]The following should solve problems that I know of:
>
> typedef std::vector<std::pair<fileno, handle::native_type>>
>     child_streams;
>
> typedef boost::function2<
>     std::pair<handle, handle>,
>         fileno,
>         const child_streams&
>     > behavior;
>
> std::pair<handle, handle> inherit(fileno fn, const child_streams&)
> {
> #ifdef _WIN32
>     handle child_end(GetStdHandle(DWORD(-10-fn)), dont_close);
> #else
>     handle child_end(fn, dont_close);
> #endif
>     handle parent_end;
>     return std::make_pair(child_end, parent_end);
> }
>
> class redirect_to
> {
>     fileno to_;
> public:
>     redirect_to(fileno to): to_(to) {}
>    std::pair<handle, handle>
>     operator()(fileno fn, const child_streams& cs)
>     {
>         BOOST_ASSERT(fn > to_);
>         child_streams::const_iterator it =
>             // find pair in cs where it->first == to_
> #ifndef _WIN32
>         handle child_end(posix_dup2(it->second, fn));
> #else
>         // ...
> #endif
>         handle parent_end;
>         return std::make_pair(child_end, parent_end);
>     }
> };
>enum std_fileno
> {
>   stdin, stdout, stderr
> };
>
> Example of usage:
>
> ctx.stderr = redirect_to(stdout);

I'm a bit confused: If you use fileno like above on Windows (0=stdin,  
1=stdout and 2=stderr) then we could also use an enumeration? At least  
your implementation of inherit makes me think so that it's not the file  
descriptor itself you are interested in but only the information whether  
the standard input, output or error stream is configured. Then there would  
be no difference between your and mine solution?

If you are interested in the actual value of fileno though (after all the  
POSIX implementation of inherit uses fileno directly) I see a couple of  
problems.

Passing fileno to a stream behavior's operator() is actually the opposite  
of what we expect a stream behavior to do: We want it to create the  
child's streams for us. For example bp::behavior::pipe should return the  
child's end of the pipe. That said it would be strange to pass a fileno to  
bp::behavior::pipe if we actually expect bp::behavior::pipe to return a  
fileno to us.

A more serious problem is that on POSIX a child process can inherit  
streams with filenos greater than 2 (eg. see  
<http://svn.boost.org/svn/boost/sandbox/SOC/2010/process/libs/process/example/file_descriptors_setup.cpp>).  
If you use bp::behavior::pipe like in the example what fileno should we  
pass to operator()? The pipe needs to know whether it is used to configure  
an input or output stream.

As on Windows handles are used to represent standard streams what should  
the first parameter passed to operator() look like? We know that the  
fileno of the standard error stream of the child process will be 2 on  
POSIX. But what will the handle look like on Windows? How will redirect_to  
know it should redirect the standard error stream? Well, fortunately  
redirect_to doesn't need to access the child's standard error stream on  
Windows but can simply dup and return the to_ stream. :-) But what kind of  
information would a handle passed to operator() provide?

Thinking it through I'm more convinced that we need to pass the  
information to a stream behavior whether it's used to configure the  
standard input, output or error stream. As filenos greater than 2 can be  
inherited on POSIX a bit field might be better as it would allow us to  
distinguish input and output streams (a filenos greater than 2 are either  
input or output streams but no standard streams).

> [...]
>> As I also think the default constructor of context shouldn't throw:
>> What if we add another optional parameter to bp::behavior::inherit? A
>> bool variable could be used to indicate whether bp::behavior::inherit
>> should throw if dup()/DuplicateHandle() fails.
>
> No, it shouldn't call dup()/DuplicateHandle() in the first place.

Convinced - the dont_close flag sounds like a good idea! I'll update the  
code on the weekend.

Boris