Re: [boost] [Boost-users] Subject: Formal Review of Proposed Boost.Process library starts tomorrow

Subject: Re: [boost] [Boost-users] Subject: Formal Review of Proposed Boost.Process library starts tomorrow
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2011-02-07 16:06:09

AMDG

Review part 2: implementation

boost/process.hpp:

boost/process/all.hpp:

boost/process/child.hpp:

The PP guard on the constructor that takes
a handle, should be
#if defined(BOOST_WINDOWS_API) || defined(BOOST_PROCESS_DOXYGEN)
so that it appears in the reference.

The map should be passed by const std::map<stream_id, handle>&
(No need to make extraneous copies.)

windows.h is not needed.

boost/process/config.hpp:

Can you use BOOST_THROW_EXCEPTION instead
of rolling your own file/line info?

boost/process/context.hpp:

boost/process/environment.hpp:

boost/process/handle.hpp:

The semantics of handle::dont_close appear to
be the same as the original boost::iostreams::file_descriptor,
in that an explicit call to close still
closes the handle.  This led to some problems,
so Daniel James changed it to have three
options.  See ticket #3517.  I'm wondering
whether similar problems are liable to
come up here.

If the handle is supposed to be closed and
construction fails, the native handle should
be closed.  Otherwise, it's difficult make
code using it exception safe.

boost/process/operations.hpp:

find_executable_in_path:

I'm not sure that asserting is the right response
when the name contains a "/."  It's not unreasonable
to expect that the name can come from outside the
program, and thus may not be valid.  If you do go
with assert, the precondition needs to be documented.

On Windows, it's possible to have a path longer than
MAX_PATH.  You should probably use the size returned
from SearchPathA to allocate a buffer that's big enough
and try again.

Cygwin executables end with .exe, just like windows.

create_child:
The context should be passed by const reference.

The requirements on Context and Arguments need to
be documented.

Is not exception safe with BOOST_POSIX_API because
if environment_to_envp throws, the memory allocated
by collection_to_argv will not be released.

I'd like everything after the child process is
started to be no-throw, so I can guarantee that
if the child is created, I can get a handle to it.
This includes returning the child object.

boost/process/pid_type.hpp:

boost/process/pipe.hpp:

Can you #include the appropriate specific
header, rather than the whole boost/asio.hpp?

boost/process/pistream.hpp:

boost/process/postream.hpp:

boost/process/process.hpp:

boost/process/self.hpp:

boost/process/status.hpp:

boost/process/stream_behavior.hpp:

boost/process/stream_ends.hpp:

boost/process/stream_id.hpp:

boost/process/stream_type.hpp:

boost/process/detail/basic_status.hpp:

Again, #include <boost/asio.hpp> is a lot more than you need.

boost/process/detail/basic_status_service.hpp:

The constructor is not exception safe.  If
starting the thread fails, the event will
be leaked.

work_thread can throw which will terminate
the process.  Is this the desired behavior
or should it be using Boost.Exception to
move the exception to a different thread?

async_wait can leak the HANDLE created
with OpenProcess.

condition variables are allowed to have
spurious wakeups.  You need to track
enough state to make sure that you don't
wake up early.

boost/process/detail/posix_helpers.hpp:

environment_to_envp and environment_to_envp
are not exception safe.

boost/process/detail/status_impl.hpp:

operation: If an argument is not used,
virtual void operator()(int /* exit_code */)
is the best way to suppress warnings.

boost/process/detail/systembuf.hpp:

I don't think that overflow is correct.
According to the MSDN documentation WriteFile
can return when:

     * The number of bytes requested is written.
     * A read operation releases buffer space on the read
       end of the pipe (if the write was blocked). For
       more information, see the Pipes section.
     * An asynchronous handle is being used and the write
       is occurring asynchronously.
     * An error occurs.

Note #2 in particular.

boost/process/detail/windows_helpers.hpp:

Can you #include <cstring> instead of <string.h>?

General:

The #include <windows/unistd.h> is
not consistent.  Sometimes there's
an extra

#else
#   error "Unsupported platform."
#endif

sometimes not.

In Christ,
Steven Watanabe