Make sure that the named pipe created for signalling the exit code is not inheritable

Make sure that the named pipe created for signalling the exit code is not inheritable

R=ager@google.com
Committed: https://code.google.com/p/dart/source/detail?r=396

[Mads Ager (google), 2011-10-13 12:40:35]

LGTM with comments.

http://codereview.chromium.org/8267006/diff/1/bin/process_win.cc
File bin/process_win.cc (right):

http://codereview.chromium.org/8267006/diff/1/bin/process_win.cc#newcode87
bin/process_win.cc:87: // NOTE: If this function returns false the handled might
have been allocated
handled -> handles

http://codereview.chromium.org/8267006/diff/1/bin/process_win.cc#newcode88
bin/process_win.cc:88: // and the caller should make sure to close then in case
of an error.
then -> them

http://codereview.chromium.org/8267006/diff/1/bin/process_win.cc#newcode126
bin/process_win.cc:126: } else if (type == kInheritWrite) {
InheritWrite and InheritNone are identical except for the pointer to a security
structure. Maybe extract a method?

http://codereview.chromium.org/8267006/diff/1/bin/process_win.cc#newcode253
bin/process_win.cc:253: CloseProcessPipe(stdin_handles);
Can't you abstract all of these closing parts into one method that take pointers
to the handle arrays and close any non-INVALID_HANDLE_VALUE handles?

http://codereview.chromium.org/8267006/diff/1/bin/process_win.cc#newcode275
bin/process_win.cc:275: if (!CreateProcessPipe(exit_handles, pipe_name,
kInheritNone)) {
kDontInheritWrite?

Or maybe kReadInherit, kWriteInherit, kWrite?

I would like to have the word write appear here somewhere. You are creating the
process pipe for writing.

[Søren Gjesse, 2011-10-13 13:33:10]

http://codereview.chromium.org/8267006/diff/1/bin/process_win.cc
File bin/process_win.cc (right):

http://codereview.chromium.org/8267006/diff/1/bin/process_win.cc#newcode87
bin/process_win.cc:87: // NOTE: If this function returns false the handled might
have been allocated
On 2011/10/13 12:40:35, Mads Ager wrote:
> handled -> handles

Done.

http://codereview.chromium.org/8267006/diff/1/bin/process_win.cc#newcode88
bin/process_win.cc:88: // and the caller should make sure to close then in case
of an error.
On 2011/10/13 12:40:35, Mads Ager wrote:
> then -> them

Done.

http://codereview.chromium.org/8267006/diff/1/bin/process_win.cc#newcode126
bin/process_win.cc:126: } else if (type == kInheritWrite) {
On 2011/10/13 12:40:35, Mads Ager wrote:
> InheritWrite and InheritNone are identical except for the pointer to a
security
> structure. Maybe extract a method?

Combined the two and used a conditional for the security descriptor argument.

http://codereview.chromium.org/8267006/diff/1/bin/process_win.cc#newcode253
bin/process_win.cc:253: CloseProcessPipe(stdin_handles);
On 2011/10/13 12:40:35, Mads Ager wrote:
> Can't you abstract all of these closing parts into one method that take
pointers
> to the handle arrays and close any non-INVALID_HANDLE_VALUE handles?

Done.

http://codereview.chromium.org/8267006/diff/1/bin/process_win.cc#newcode275
bin/process_win.cc:275: if (!CreateProcessPipe(exit_handles, pipe_name,
kInheritNone)) {
On 2011/10/13 12:40:35, Mads Ager wrote:
> kDontInheritWrite?
> 
> Or maybe kReadInherit, kWriteInherit, kWrite?
> 
> I would like to have the word write appear here somewhere. You are creating
the
> process pipe for writing.

The pipe is always created with both read and write ends. The type only
specifies whether one of these ends should be inheritable by the child. So I
think the current names are OK.