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

Subject: Re: [boost] [gsoc] Boost.Process done)
From: Jeremy Maitin-Shepard (jeremy_at_[hidden])
Date: 2010-09-11 18:53:17

On 09/11/2010 02:47 PM, Boris Schaeling wrote:
> On Sat, 11 Sep 2010 22:51:25 +0200, Jeremy Maitin-Shepard
> <jeremy_at_[hidden]> wrote:
>
>> On 09/11/2010 01:32 PM, Boris Schaeling wrote:
>>> Indeed. But as context doesn't support arbitrary file descriptors those
>>> must be configured differently anyway.
>>
>> I'm suggesting that it should. From the library's perspective, there
>> is no difference between configuring the child's file descriptors 0,
>> 1, and 2, and configuring any other file descriptors, at least on
>> POSIX, and given that it is useful for the user, it might as well be
>> exposed. (Of course, on Windows, things are different, but I think you
>> have just as elegant an interface and still allow this flexibility for
>> POSIX while retaining portable syntax.)
>
> I'm not really convinced of adding platform-specific features. We had
> POSIX- and Windows-only classes and functions in previous versions of
> the library (in fact that's how my Boost.Process version 0.3 looked
> like). The implementation of the new version is by far less bloated and
> much cleaner.
>
> But maybe we can add another extension point? In fact it's not required
> to subclass context as create_child() is a function template. You can
> create another context class with new member variables and pass it to
> create_child(). The question is then what exactly would the extension
> point need to look like and how would it differ from context::setup()
> which is already provided today?

I agree that it is undesirable for the library to have explicit 
support/a wrapper interface for every single platform-specific feature. 
  For POSIX, the existing function callback interface is exactly the 
right interface for supporting things like setuid, etc.  (As I've 
mentioned before, though, I'd say the Windows extension interface is a 
bit lacking, though.)

The exception, however, is support for file descriptor mapping, for 
which the POSIX extension interface is not sufficient, and which is 
obviously extremely closely related to handling of the "standard 
streams".  In particular, I'd say that with the current interface, 
anyone that needs to do file descriptor mappings for file numbers > 2 
basically shouldn't use boost.process.

>
>>> Please have a look at
>>> <http://svn.boost.org/svn/boost/sandbox/SOC/2010/process/libs/process/example/file_descriptors_setup.cpp>.
>>>
>>> There are two pipes used to create file descriptors 3 and 4 which should
>>> be inherited by the child process. As the pipe stream behavior is used
>>> explicitly the user knows if 3 and 4 should be read or write ends of the
>>> pipe. The user can pass the information to the pipe stream behavior
>>> while the context indeed can't (in this example there is still a boolean
>>> value used).
>>
>> I saw that example, but it hardly seemed like a very good exposition
>> of the convenience provided by the library. (Also, as I alluded to in
>> some
>
> I agree. It's a bit strange to use stream behaviors like this. But then
> stream behaviors were really invented to create a platform-independent
> way of configuring standard streams and not to support platform-specific
> features.
>
>> of my other example code snippets and as has been discussed a bit by
>> others, I think that using an std::pair as the return value for those
>> type of things is a bad idea. Instead, read_end and write_end, or
>> parent_end and child_end, depending on the situation, should be used.)
>
> We need both ends in create_child() as the function returns a child
> object. It is the one used by the parent process to possibly communicate
> with the child process: One end is passed to the child process and the
> other end remains in the parent process so it can communicate with the
> child process.

I was just suggesting the use of a custom struct rather than std::pair, 
so that you would have named members rather than first and second, to 
avoid confusion about the meaning of first and second.

However, in some other e-mails, I suggest that boost process shouldn't 
itself hold on to the parent end of pipes and store them in 
child_process instances for the purpose of providing get_stdin(), etc. 
Instead, the parent end of the pipe is returned to the caller in the 
course of setting up the mapping, and then boost process doesn't need to 
do anything with it after that.  I think this leads to a cleaner 
interface, since with the current interface, get_stdin, etc. are always 
present in child_process but only meaningful if you happened to have 
configured a pipe on the corresponding file number.

Also, the current interface seems to preclude setting up a pipe between 
two child processes.

Consider the following syntax:

context ls_ctx, less_ctx;
pipe(ls_ctx[bp::stdout], less_ctx[bp::stdin]);
inherit(less_ctx[bp::stdout]);
inherit(less_ctx[bp::stderr]);

Note: bp::std{in,out,err} would be enum values, and operator[] would 
have an argument of enum type on Windows, and type int on POSIX, as 
suggested by Ilya.

operator[] would return an object, e.g. fd_map_t that stores the 
argument and a reference to the context, and would support operator= to 
assign a mapping (pipe, inherit, etc. would internally invoke this 
operator=).

So you could also do:

file_descriptor_t ls_3 = output_pipe(ls_ctx[3]); // ls_ctx[3] is 
assigned to write end, ls_3 refers to read end.

Potentially you could add some safety and/or syntactic convenience by 
having types like readable_file_descriptor_t, 
writable_file_descriptor_t, readwrite_file_descriptor_t in addition to 
file_descriptor_t, with the obvious implicit move conversions allowed, 
and explicit move conversions allowed between any of them.  Then, you 
could have bp::pipe() return a struct e.g. pipe_t that contains both 
ends, and define various operators like:

write_file_descriptor_t operator>>(pipe_t &&, fd_map_t);
write_file_descriptor_t operator<<(fd_map_t, pipe_t &&);
read_file_descriptor_t operator>>(fd_map_t, pipe_t &&);
read_file_descriptor_t operator<<(pipe_t &&, fd_map_t);
void operator>>(fd_map_t, write_file_descriptor_t &&);
void operator<<(fd_map_t, read_file_descriptor_t &&);

Then you could have:

ls_ctx[bp::stdout] >> bp::pipe() >> less_ctx[bp::stdin];

write_file_descriptor_t ls_3 = ls_ctx[bp::stdout] >> bp::pipe();

The additional advantage of tagging the file descriptor types with 
read/write is that it can provide additional type safety if you then use 
them with either boost iostreams or boost asio.

Note: My imagination might be running a bit too wild with these last 
syntactic suggestions, but in any case I think it demonstrates that you 
can support arbitrary file descriptor mappings on POSIX without 
sacrificing the quality of the portable interface.

If you wanted to, you could try to prevent the user from assigning the 
wrong direction stream to std{out,in,err} using typing (e.g. 
bp::stdout,bp::stderr would have a type different from that of 
bp::stdin, and there would be a total of 3 versions of operator[] on 
POSIX, and two versions on Windows), but I don't think it is really 
necessary.