code review 4306041: syscall: StartProcess fixes for windows

syscall: StartProcess fixes for windows

- StartProcess will work with relative (to attr.Dir, not
  current directory) executable filenames
- StartProcess will only work if executable filename points
  to the real file, it will not search for executable in the
  $PATH list and others (see CreateProcess manual for details)
- StartProcess argv strings can contain any characters

[brainman, 2011-03-22 06:22:18]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2011-03-22 17:12:25]

http://codereview.appspot.com/4306041/diff/4001/src/pkg/exec/exec_test.go
File src/pkg/exec/exec_test.go (right):

http://codereview.appspot.com/4306041/diff/4001/src/pkg/exec/exec_test.go#new...
src/pkg/exec/exec_test.go:140: "2 \\^",
use raw strings to avoid backslashes.

http://codereview.appspot.com/4306041/diff/4001/src/pkg/exec/exec_test.go#new...
src/pkg/exec/exec_test.go:166: t.Errorf("read: got %q expect %q", string(buf),
expect)
you don't need string(buf) here - just buf will do

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows.go
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:17: func escape(s string) string {
a little description here would be nice too.  cite the reference (thanks for
that) but make it a citation not the full comment.
what's going on here is intricate enough it deserves explanation.

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:22: haswspaces := false
hasSpaces

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:75: func makeCmdLine(args []string) string {
again, a tiny comment would be good

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:87: //Last bytes are two unicode nulls, or four
null bytes.
put a space after //

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:119: func getFullPath(name string) (path string,
err int) {
again, comments please

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:228: // argv0 relative to current directory,
and, only once new
s/new/the new/

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:230: // for that difference here by making argc0
absolute.
s/argc0/argv0/

[brainman, 2011-03-23 00:20:08]

Thank you for review. PTAL.

Alex

http://codereview.appspot.com/4306041/diff/4001/src/pkg/exec/exec_test.go
File src/pkg/exec/exec_test.go (right):

http://codereview.appspot.com/4306041/diff/4001/src/pkg/exec/exec_test.go#new...
src/pkg/exec/exec_test.go:140: "2 \\^",
On 2011/03/22 17:12:25, r wrote:
> use raw strings to avoid backslashes.

Done.

http://codereview.appspot.com/4306041/diff/4001/src/pkg/exec/exec_test.go#new...
src/pkg/exec/exec_test.go:166: t.Errorf("read: got %q expect %q", string(buf),
expect)
On 2011/03/22 17:12:25, r wrote:
> you don't need string(buf) here - just buf will do

Done.

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows.go
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:17: func escape(s string) string {
On 2011/03/22 17:12:25, r wrote:
> a little description here would be nice too.  cite the reference (thanks for
> that) but make it a citation not the full comment.
> what's going on here is intricate enough it deserves explanation.

Done.

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:22: haswspaces := false
On 2011/03/22 17:12:25, r wrote:
> hasSpaces

Done.

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:75: func makeCmdLine(args []string) string {
On 2011/03/22 17:12:25, r wrote:
> again, a tiny comment would be good

Done.

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:87: //Last bytes are two unicode nulls, or four
null bytes.
On 2011/03/22 17:12:25, r wrote:
> put a space after //

Done.

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:119: func getFullPath(name string) (path string,
err int) {
On 2011/03/22 17:12:25, r wrote:
> again, comments please

Done.

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:228: // argv0 relative to current directory,
and, only once new
On 2011/03/22 17:12:25, r wrote:
> s/new/the new/

Done.

http://codereview.appspot.com/4306041/diff/4001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:230: // for that difference here by making argc0
absolute.
On 2011/03/22 17:12:25, r wrote:
> s/argc0/argv0/

Done.

[r, 2011-03-23 00:28:50]

http://codereview.appspot.com/4306041/diff/9001/src/pkg/exec/exec_test.go
File src/pkg/exec/exec_test.go (right):

http://codereview.appspot.com/4306041/diff/9001/src/pkg/exec/exec_test.go#new...
src/pkg/exec/exec_test.go:147:
"BEGIN{printf(\"%s|%s|%s\",ARGV[1],ARGV[2],ARGV[3])}",
this is fine, but you could use a raw string here too

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows.go
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:24: //   but only if there are whitespaces
inside s.
s/are whitespaces/is space/ or /are spaces/

white space is a non-concept. as i review this in codereview, white space is
green!  just say call it space - everyone knows.

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:30: hasSpaces := false
if you call this hasSpace, it's more generic (see above)

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:81: }
this is a nasty function. i'd really like a test. unfortunately, syscall doesn't
have and can't have proper tests.  but... i wonder if this is a useful enough
function to go in the windows piece of package os, and then it could be tested
there.

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:83: // Make command line out of args, by
escaping "special" characters
// makeCmdLine does this ...
etc. throughout.
even though this isn't a published name, why not be consistent? also if it
becomes published, the comment is ready to roll.

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:96: // Env block is a sequence of null
terminated strings followed by a null.
// createEnvBlock does this ...

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:139: // Windows is asking for bigger buffer.
if you're going to do this, you don't need the default to be so huge. 100 should
be plenty. 50 even, probably.

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:200: }
gawd, windows paths are clumsy

[brainman, 2011-03-23 01:15:25]

PTAL

http://codereview.appspot.com/4306041/diff/9001/src/pkg/exec/exec_test.go
File src/pkg/exec/exec_test.go (right):

http://codereview.appspot.com/4306041/diff/9001/src/pkg/exec/exec_test.go#new...
src/pkg/exec/exec_test.go:147:
"BEGIN{printf(\"%s|%s|%s\",ARGV[1],ARGV[2],ARGV[3])}",
Changed to use raw string.

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows.go
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:24: //   but only if there are whitespaces
inside s.
I want both spaces and tabs included here. Changed.

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:30: hasSpaces := false
Changed to hasSpace.

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:81: }
I agree, it is bit of a twist, thus I have all these tryargs to test it in exec
package. I don't think this function is any use, but here (for StartProcess). We
could always move it if need to be.

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:83: // Make command line out of args, by
escaping "special" characters
On 2011/03/23 00:28:50, r wrote:
> // makeCmdLine does this ...
> etc. throughout.
> even though this isn't a published name, why not be consistent? also if it
> becomes published, the comment is ready to roll.

Done.

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:96: // Env block is a sequence of null
terminated strings followed by a null.
On 2011/03/23 00:28:50, r wrote:
> // createEnvBlock does this ...

Done.

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:139: // Windows is asking for bigger buffer.
Calling GetFullPathName is not cheap also, so I'm trying to avoid that. 100 it
is.

http://codereview.appspot.com/4306041/diff/9001/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:200: }
No comment! <g>

[r, 2011-03-23 21:07:32]

LGTM
these are all just commentary

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows.go
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:83: // makeCmdLine builds command line out of
args, by escaping "special"
s/command/a command/
s/,//

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:84: // characters inside every argument and
concatenating them with spaces.
characters and joining the arguments with spaces.

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:96: // createEnvBlock converts envv array of
environment strings into
s/envv /an/

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:97: // representation required by CreateProcess:
sequence of null
s/rep/the rep/
s/sequence/a &/
s/null/NUL/  (null is a pointer, NUL is a character)

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:98: // terminated strings followed by a null.
s/null/nil/

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:99: // Last bytes are two unicode nulls, or four
null bytes.
s/null/NUL/g
s/unicode/UCS-2/

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:241: // Windows CreateProcess assumes the
opposite, it looks for
s/,/:/

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:242: // argv0 relative to current directory,
and, only once the new
s/current/the current/

[brainman, 2011-03-24 00:20:32]

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

syscall: StartProcess fixes for windows

- StartProcess will work with relative (to attr.Dir, not
  current directory) executable filenames
- StartProcess will only work if executable filename points
  to the real file, it will not search for executable in the
  $PATH list and others (see CreateProcess manual for details)
- StartProcess argv strings can contain any characters

R=golang-dev, r
CC=golang-dev
http://codereview.appspot.com/4306041

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows.go
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:83: // makeCmdLine builds command line out of
args, by escaping "special"
On 2011/03/23 21:07:32, r wrote:
> s/command/a command/
> s/,//

Done.

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:84: // characters inside every argument and
concatenating them with spaces.
On 2011/03/23 21:07:32, r wrote:
> characters and joining the arguments with spaces.

Done.

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:96: // createEnvBlock converts envv array of
environment strings into
On 2011/03/23 21:07:32, r wrote:
> s/envv /an/

Done.

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:97: // representation required by CreateProcess:
sequence of null
On 2011/03/23 21:07:32, r wrote:
> s/rep/the rep/
> s/sequence/a &/
> s/null/NUL/  (null is a pointer, NUL is a character)

Done.

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:98: // terminated strings followed by a null.
On 2011/03/23 21:07:32, r wrote:
> s/null/nil/

Done.

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:99: // Last bytes are two unicode nulls, or four
null bytes.
On 2011/03/23 21:07:32, r wrote:
> s/null/NUL/g
> s/unicode/UCS-2/

Done.

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:241: // Windows CreateProcess assumes the
opposite, it looks for
On 2011/03/23 21:07:32, r wrote:
> s/,/:/

Done.

http://codereview.appspot.com/4306041/diff/14001/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:242: // argv0 relative to current directory,
and, only once the new
On 2011/03/23 21:07:32, r wrote:
> s/current/the current/

Done.