On 09/10/2010 03:57 PM, Boris Schaeling wrote:
On Fri, 10 Sep 2010 05:02:54 +0200, Jeremy Maitin-Shepard <jeremy@jeremyms.com> wrote:
Jeremy,
I'll go through your feedback quickly as it's pretty late here. :)
[...] - The std::istream/std::ostream interface should be decoupled from the facility for retrieving the file descriptor (int) or Windows HANDLE, to avoid the overhead of allocating a streambuf for those users that wish to access the file descriptor/HANDLE directly or use it with asio. Instead, the std::istream/std::ostream interface to a file descriptor/HANDLE could be provided as a separate, completely independent facility (designed so that exactly the same syntax could be used on both Windows and POSIX), and likewise a separate facility for providing an asio interface to a file descriptor/HANDLE (which also allows exactly the same syntax to be used on POSIX and Windows).
Can you show some sample code you would like to write? Then I think I understand better how Boost.Process would need to be changed.
See below.
- It seems it would be cleaner for the parent side of these pipes to be returned in the course of specifying that pipes should be used (in the current terminology, in the course of specifying the pipe stream behavior). This prevents context from being reusable, but it seems that it would be reusable only a very limited set of circumstances anyway. (E.g. if the context::setup method needs additional invocation-specific
Not sure what you mean - can you show some sample code again? :)
file_descriptor_t stdout_pipe = output_pipe(ctx, bp::stdout); file_descriptor_t stdin_pipe = input_pipe(ctx, bp::stdin) (file_descriptor_t would be some type that manages the lifetime of the int/HANDLE.) bp::std{in,out,err} would be enum values, and on posix, you could also use a numeric fd. output_pipe and input_pipe would be convenience functions that couple the creation of a pipe with setting up a file descriptor redirection on ctx, e.g. something like file_descriptor_t output_pipe(context &ctx, fileno fd) { pipe_t p = create_pipe(); ctx.map_fd(fd, boost::move(p.write_end)); return boost::move(p.read_end); } I'm not sure what the status of boost::move is, but... Alternatively, file_descriptor_t could be reference counted, though I like moveable better. You could then use asio, boost iostreams, or something else to interact with these file_descriptors.
parameters, etc.) Furthermore, it seems inconsistent that certain things are part of the "reusable" context, like the environment and process_name, but other things, like the executable path and arguments, are not.
So far reusing a context means launching the very same application a second time. This didn't work first with the stream behavior class hierarchy but works now with the delegates.
It seems that it is questionable how useful this would actually be in most applications, and with the added overhead of redirection through boost::function, it might well actually be more efficient to just obtain the same effect by writing a function that takes care of creating the context and calling create_process, etc.
- On POSIX, due to the complicated semantics of waitpid, etc. and SIGCHLD, and the general desire to avoid leaving around zombie child processes, any code that deals with child processes needs to coordinate with all other code that deals with child processes. The library should explicitly document what it does with respect to SIGCHLD signals and waiting on child processes (and possibly change the behavior if the current behavior doesn't handle all cases properly), so that other code can correctly interact with it.
I agree that the documentation should be improved. It was created from scratch for Boost.Process 0.4 and doesn't cover yet really everything.
- As was observed in the discussion regarding setting up stdin/stdout/stderr file descriptors on POSIX, it is somewhat tricky to correctly set up child file descriptors. As I believe it is fairly common on POSIX to want to set up child file descriptors other than 0, 1, and 2, it would be best for the library to provide explicit support for this, rather than forcing the user to implement it manually in context::setup. A reasonable interface might be e.g.: context::map_file_descriptor(int child_fileno, file_descriptor_t existing_descriptor);
Once we start to support platform-specific features the question is where do we end? The context class in previous Boost.Process versions grew more and more as support for uid, euid, gid, eguid, chroot etc. was added. Instead of arguing about each and every feature whether it's justified to hardcode it into the library we decided it might be better to support cross-platform features only and provide extension points.
I don't suggest that Boost.Process should provide an interface to specific Windows features; however, I believe the current Windows extension interface is not sufficiently powerful, whereas the POSIX one is. However, I think file descriptor setup on POSIX is a special case, because it is something the library is already doing anyway (simply restricted to 0, 1, and 2), and is also something that is fairly complicated to implement correctly, in contrast to e.g. setuid which just involves a single library call.
[snip]
- On Windows, it seems there should be both a char and wchar_t versionof everything that involves characters. I suggest that the wchar_t version simply not exist on POSIX, as it wouldn't make much sense nor would it be useful.
I don't remember anymore all the details which made it difficult to add support for wide chars in previous Boost.Process versions. But this is something I still want to look into (probably after everything else has been settled).
It seems it could either be done with templates, or even just using the preprocessor. Somehow supporting wchar_t on POSIX would naturally be a lot more difficult, but it wouldn't be useful anyway. As an additional comment that I didn't mention before, and seems to be related to some recent comments made in this thread regarding the fact that POSIX can inherit more than just stdin,stdout,stderr, there needs to be a facility on POSIX for specifying the default behavior for file descriptors not otherwise specified, e.g. either inherit or close.