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

Subject: Re: [boost] [gsoc] Boost.Process done
From: Ilya Sokolov (ilyasokol_at_[hidden])
Date: 2010-09-09 19:33:53

On Wed, 08 Sep 2010 23:20:01 +0200
"Boris Schaeling" <boris_at_[hidden]> wrote:

> On Wed, 08 Sep 2010 03:37:52 +0200, Ilya Sokolov
> <ilyasokol_at_[hidden]> wrote:
> 
> > [...]
> >> I'd appreciate if you could comment on the changes. Especially
> >> have a look at:
> >>
> >> * this signature: std::pair<handle, handle> (bool).
> >
> > It would be better if delegate would accept fileno instead of bool
> > indicating input/output. bp::behavior::inherit seems almost
> > indistinguishable from the redirect_to.
> 
> Which fileno should be passed? stdin/stdout/stderr of the parent
> process?

typedef int fileno;

> Maybe we have a similar idea because I also think it would be better
> to distinguish standard output and standard error. This could be done
> with an enumeration and could maybe help to create a better
> redirect_to stream behavior. The problem with redirect_to though is
> that it depends on another stream behavior. I don't know whether
> Boost.Process should support dependencies between stream behaviors?

Yes, at least for redirect_to_stdout behavior. It is fairly common use
case.

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_);
#ifndef _WIN32
    handle child_end(posix_dup2(to_, 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);

> [snip]
>
 >> * The context class still has a default constructor only which  
> >> initializes the stream behaviors to inherit standard streams. This
> >> is a problem for Windows GUI applications as they don't have
> >> standard streams (or at least they are closed). While the Named
> >> Constructor Idiom has been proposed what exactly should the done?
> >> Should the default constructor be private and developers must
> >> explicitly use a factory function to create a context object?
> >> Shall the factory function initialize all standard streams with
> >> the same behavior? Shall we provide a factory function which
> >> expects three parameters to initialize the standard streams?
> >> What's convenient and flexible and not too confusing?
> >
> > I expect that if some std stream is closed in the calling process,
> > the default constructor of the context class doesn't throw anything.
> > As I see it, the bp::handle should have a "dont-close" flag and
> > bp::behavior::inherit shouldn't call dup() or DuplicateHandle().
> 
> The reason why bp::behavior::inherit calls dup()/DuplicateHandle() is
> that a parent closes a child's handles (and a child a parent's
> handles; this is done in create_child()). Otherwise both processes
> would have a read and write end open when a pipe is used. Without a
> dup()/DuplicateHandle() a parent would close its own standard streams

It wouldn't if you add "don't-close" flag.

> if it wants the child to inherit them. I think it also makes logical
> sense to automatically close the streams of the other process?
> 
> 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.

> By default it throws -
> but it doesn't when used by the default constructor of context?
> 
> Anyway, enough ideas for now. :)

I still have some )