code review 5986044: os/exec: close all internal descriptors when Cmd.Start(...

os/exec: close all internal descriptors when Cmd.Start() fails.

This closes any internal descriptors (pipes, etc) that Cmd.Start() had
opened before it failed.

Fixes issue 3468.

[bdd, 2012-04-05 01:50:16]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[iant, 2012-04-20 20:41:10]

http://codereview.appspot.com/5986044/diff/3001/src/pkg/os/exec/exec.go
File src/pkg/os/exec/exec.go (right):

http://codereview.appspot.com/5986044/diff/3001/src/pkg/os/exec/exec.go#newco...
src/pkg/os/exec/exec.go:210: all = -1
Perhaps all = afterStart|afterWait.

http://codereview.appspot.com/5986044/diff/3001/src/pkg/os/exec/exec.go#newco...
src/pkg/os/exec/exec.go:213: func (c *Cmd)
closeInternalDescriptors(descriptorSet int) {
s/descriptorSet/which/

[bradfitz, 2012-04-20 20:46:42]

http://codereview.appspot.com/5986044/diff/3001/src/pkg/os/exec/exec.go
File src/pkg/os/exec/exec.go (right):

http://codereview.appspot.com/5986044/diff/3001/src/pkg/os/exec/exec.go#newco...
src/pkg/os/exec/exec.go:210: all = -1
On 2012/04/20 20:41:10, iant wrote:
> Perhaps all = afterStart|afterWait.


We don't even need these constants.

http://codereview.appspot.com/5986044/diff/3001/src/pkg/os/exec/exec.go#newco...
src/pkg/os/exec/exec.go:213: func (c *Cmd)
closeInternalDescriptors(descriptorSet int) {
func (c *Cmd) closeDescriptors(closers ...[]io.Closer) {
   for _, s := range closers {
      for _, fd := range s {
           fd.Close()
      }
   }
}

...

c.closeDescriptors(c.closeAfterStart, c.closeAfterWait)


But it could even just be two lines and simplify the close signature:

c.closeDescriptors(c.closeAfterStart)
c.closeDescriptors(c.closeAfterWait)

[bdd, 2012-04-24 01:12:40]

Hello golang-dev@googlegroups.com, iant@golang.org, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[iant, 2012-04-27 17:21:02]

LGTM

[iant, 2012-04-27 22:46:53]

*** Submitted as http://code.google.com/p/go/source/detail?r=9d046474e95a ***

os/exec: close all internal descriptors when Cmd.Start() fails.

This closes any internal descriptors (pipes, etc) that Cmd.Start() had
opened before it failed.

Fixes issue 3468.

R=golang-dev, iant, bradfitz
CC=golang-dev
http://codereview.appspot.com/5986044

Committer: Ian Lance Taylor <iant@golang.org>