code review 1578041: add Windows ForkExec, Syscall12

add Windows ForkExec, Syscall12
        Updated script that generates syscalls to use them if need be.
        Added windows ForkExec to windows.  Needs tracing, env, and Fd added still.
	Renamed syscall/exec.go to exec_unix.go.

[kardia, 2010-06-07 03:56:05]

Hello brainman, rsc1 (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc1, 2010-06-07 20:01:07]

thanks for doing this.  please make the first line
of the CL description follow the guidelines at
http://golang.org/doc/contribute.html.  for example:

syscall: add Windows ForkExec, Syscall12, Syscall15

http://codereview.appspot.com/1578041/diff/2001/3004
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/1578041/diff/2001/3004#newcode54
src/pkg/syscall/exec_windows.go:54: // Convert array of string to array
looks like it was copied but is unused

http://codereview.appspot.com/1578041/diff/2001/3004#newcode64
src/pkg/syscall/exec_windows.go:64: func CloseOnExec(fd int) { fcntl(fd,
F_SETFD, FD_CLOEXEC) }
on Windows this should be a no-op

http://codereview.appspot.com/1578041/diff/2001/3004#newcode66
src/pkg/syscall/exec_windows.go:66: func SetNonblock(fd int, nonblocking bool)
(errno int) {
This is probably a no-op too, at least until there's a package net.

http://codereview.appspot.com/1578041/diff/2001/3004#newcode96
src/pkg/syscall/exec_windows.go:96: StringToUTF16Ptr("\""+argv0+"\"
"+stringJoin(argv, " ")),
I'd like to see quotes around every argument.
Is there some syntax to escape existing quotes?

[brainman, 2010-06-08 07:18:24]

On 2010/06/07 03:56:05, kardia wrote:
> Hello brainman, rsc1 (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change.

Thanks for making the effort.

I tried to write small program similar to src\pkg\exec\exec_test.go, like so:

	cmd, err := exec.Run("c:\\winnt\\system32\\netstat.exe", []string{"netstat"},
nil, "",
		exec.DevNull, exec.Pipe, exec.DevNull)
	if err != nil {
		abortf("run: %s\n", err)
	}
	buf, err := ioutil.ReadAll(cmd.Stdout)
	if err != nil {
		abortf("read: %s\n", err)
	}
	fmt.Printf("buf: %v\n", buf)
	if err = cmd.Close(); err != nil {
		abortf("close: %s\n", err)
	}

but it fails with

C:\TMP>\\sos\pub\test
run: open /dev/null: The system cannot find the path specified.

Do you have a small program that runs? 


Alex

[kardia, 2010-06-08 13:43:38]

>     cmd, err := exec.Run("c:\\winnt\\system32\\netstat.exe",
> []string{"netstat"}, nil, "",
>         exec.DevNull, exec.Pipe, exec.DevNull)

Right now the working dir is mandatory.  Give it a real path, such as 
"C:\\".  If you don't it will complain.  For consistency, this is one of 
the issues that might need to tested for.  I'll add a comment to the 
code for now.

[kardia, 2010-06-08 14:09:21]

Hello brainman, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[kardia, 2010-06-08 14:17:04]

On 2010/06/08 14:09:21, kardia wrote:
> Hello brainman, rsc (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Alex, here's a test program.  You'll probably need to change windows to winnt.

package main

import "os"
import "fmt"

func main() {
	fmt.Printf("A\n")
	pid, err := os.ForkExec("c:\\windows\\system32\\netstat.exe", []string { ""},
[]string{""}, "c:\\windows\\system32\\", []*os.File{})
	fmt.Printf("B\n")
	if err != nil {
		fmt.Printf("Error: %s", err.String())
	}
	fmt.Printf("C\n")
	fmt.Printf("PID: %d\n", pid)
}

[kardia, 2010-06-08 14:25:27]

Hello brainman, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-06-09 09:18:34]

http://codereview.appspot.com/1578041/diff/14001/11009
File src/pkg/runtime/windows/syscall.goc (right):

http://codereview.appspot.com/1578041/diff/14001/11009#newcode49
src/pkg/runtime/windows/syscall.goc:49: func Syscall15(trap uintptr, a1 uintptr,
a2 uintptr, a3 uintptr, a4 uintptr, a5 uintptr, a6 uintptr, a7 uintptr, a8
uintptr, a9 uintptr, a10 uintptr, a11 uintptr, a12 uintptr, a13 uintptr, a14
uintptr, a15 uintptr) (r1 uintptr, r2 uintptr, lasterr uintptr) {
Do you really need Syscall15?

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

http://codereview.appspot.com/1578041/diff/14001/11012#newcode102
src/pkg/syscall/exec_windows.go:102: startupInfo := new(StartupInfo)
Aren't you supposed to at lease set startupInfo.Cb?

http://codereview.appspot.com/1578041/diff/14001/11012#newcode109
src/pkg/syscall/exec_windows.go:109: // 0x00000400 = UNICODE env
Instead of the comment, I would have const:

CREATE_UNICODE_ENVIRONMENT = 0x00000400

http://codereview.appspot.com/1578041/diff/14001/11014
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1578041/diff/14001/11014#newcode135
src/pkg/syscall/syscall_windows.go:135: //sys	WaitForSingleObject(handle int32,
waitMilliseconds uint32) (newlowoffset uint32, errno int)
[failretval=0xffffffff]
I think, these should be:

//sys   CreateProcess(appName *uint16, commandLine *uint16, procSecurity *byte,
threadSecurity *byte, inheritHandles uint32, creationFlags uint32, env *byte,
currentDir *uint16, startupInfo *StartupInfo, outProcInfo *ProcessInformation) 
(ok bool, errno int) = CreateProcessW
//sys   GetStartupInfo(startupInfo *StartupInfo) = GetStartupInfoW
//sys   WaitForSingleObject(handle int32, waitMilliseconds uint32) (event
uint32, errno int) [failretval=0xffffffff]

I would even make it CreateProcess(..., inheritHandles bool, ...), but you would
have to change mksyscall_windows.sh to handle bool input parameters.

http://codereview.appspot.com/1578041/diff/14001/11014#newcode459
src/pkg/syscall/syscall_windows.go:459: //CloseHandle(handle)
Why the comment?

http://codereview.appspot.com/1578041/diff/14001/11016
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/1578041/diff/14001/11016#newcode158
src/pkg/syscall/ztypes_windows_386.go:158: type StartupInfo struct {
Get all types right as much as you can:

DWORD = uint32
LPTSTR = *unit16
LPBYTE = *byte
HANDLE = int32

http://codereview.appspot.com/1578041/diff/14001/11016#newcode158
src/pkg/syscall/ztypes_windows_386.go:158: type StartupInfo struct {
I say, keep field names as close to doco as possible, for example:

  DWORD  Cb;
  LPTSTR Reserved;
  LPTSTR Desktop;
  LPTSTR Title;
  DWORD  X;
  DWORD  Y;
  DWORD  XSize;
  DWORD  YSize;
  DWORD  XCountChars;
  DWORD  YCountChars;
  DWORD  FillAttribute;
  DWORD  Flags;
  WORD   ShowWindow;
  WORD   Reserved2;
  LPBYTE Reserved2_;
  HANDLE StdInput;
  HANDLE StdOutput;
  HANDLE StdError;

http://codereview.appspot.com/1578041/diff/14001/11016#newcode179
src/pkg/syscall/ztypes_windows_386.go:179: type ProcessInformation struct {
As above:

  HANDLE Process;
  HANDLE Thread;
  DWORD  ProcessId;
  DWORD  ThreadId;

[kardia, 2010-06-11 01:32:15]

> http://codereview.appspot.com/1578041/diff/14001/11009#newcode49
> src/pkg/runtime/windows/syscall.goc:49: func Syscall15(trap uintptr, a1
uintptr,
> a2 uintptr, a3 uintptr, a4 uintptr, a5 uintptr, a6 uintptr, a7 uintptr, a8
> uintptr, a9 uintptr, a10 uintptr, a11 uintptr, a12 uintptr, a13 uintptr, a14
> uintptr, a15 uintptr) (r1 uintptr, r2 uintptr, lasterr uintptr) {
> Do you really need Syscall15?

Syscall15 is not used in Go's base packages.  Removed.


> http://codereview.appspot.com/1578041/diff/14001/11012#newcode102
> src/pkg/syscall/exec_windows.go:102: startupInfo := new(StartupInfo)
> Aren't you supposed to at lease set startupInfo.Cb?

GetStartupInfo does this.

> http://codereview.appspot.com/1578041/diff/14001/11012#newcode109
> src/pkg/syscall/exec_windows.go:109: // 0x00000400 = UNICODE env
> Instead of the comment, I would have const:
> 
> CREATE_UNICODE_ENVIRONMENT = 0x00000400

Done.


> http://codereview.appspot.com/1578041/diff/14001/11014#newcode135
> src/pkg/syscall/syscall_windows.go:135: //sys	WaitForSingleObject(handle
int32,
> waitMilliseconds uint32) (newlowoffset uint32, errno int)
> [failretval=0xffffffff]
> I think, these should be:
> 
> //sys   CreateProcess(appName *uint16, commandLine *uint16, procSecurity
*byte,
> threadSecurity *byte, inheritHandles uint32, creationFlags uint32, env *byte,
> currentDir *uint16, startupInfo *StartupInfo, outProcInfo *ProcessInformation)

> (ok bool, errno int) = CreateProcessW
> //sys   GetStartupInfo(startupInfo *StartupInfo) = GetStartupInfoW
> //sys   WaitForSingleObject(handle int32, waitMilliseconds uint32) (event
> uint32, errno int) [failretval=0xffffffff]

In CreateProcess, changed uint16 to *uint16, changed calling 0 to nil.
Other functions look good to me.

> I would even make it CreateProcess(..., inheritHandles bool, ...), but you
would
> have to change mksyscall_windows.sh to handle bool input parameters.

That would be nice.  Maybe later.

> http://codereview.appspot.com/1578041/diff/14001/11014#newcode459
> src/pkg/syscall/syscall_windows.go:459: //CloseHandle(handle)
> Why the comment?

Note to myself (or whoever) in implementing a wait for process termination.
Removed.

> http://codereview.appspot.com/1578041/diff/14001/11016#newcode158
> src/pkg/syscall/ztypes_windows_386.go:158: type StartupInfo struct {
> Get all types right as much as you can:
> 
> DWORD = uint32
> LPTSTR = *unit16
> LPBYTE = *byte
> HANDLE = int32
>
> http://codereview.appspot.com/1578041/diff/14001/11016#newcode158
> src/pkg/syscall/ztypes_windows_386.go:158: type StartupInfo struct {
> I say, keep field names as close to doco as possible, for example:
> 
>   DWORD  Cb;
>   LPTSTR Reserved;
>   LPTSTR Desktop;
>   LPTSTR Title;
>   DWORD  X;
>   DWORD  Y;
>   DWORD  XSize;
>   DWORD  YSize;
>   DWORD  XCountChars;
>   DWORD  YCountChars;
>   DWORD  FillAttribute;
>   DWORD  Flags;
>   WORD   ShowWindow;
>   WORD   Reserved2;
>   LPBYTE Reserved2_;
>   HANDLE StdInput;
>   HANDLE StdOutput;
>   HANDLE StdError;
> 
> http://codereview.appspot.com/1578041/diff/14001/11016#newcode179
> src/pkg/syscall/ztypes_windows_386.go:179: type ProcessInformation struct {
> As above:
> 
>   HANDLE Process;
>   HANDLE Thread;
>   DWORD  ProcessId;
>   DWORD  ThreadId;

Made names match windows symbols.

[kardia, 2010-06-11 01:33:20]

Hello brainman, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-06-11 11:16:37]

http://codereview.appspot.com/1578041/diff/11018/26004
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/1578041/diff/11018/26004#newcode109
src/pkg/syscall/exec_windows.go:109: // 0x00000400 = UNICODE env
Do not need the comment about UNICODE now.

http://codereview.appspot.com/1578041/diff/11018/26006
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1578041/diff/11018/26006#newcode132
src/pkg/syscall/syscall_windows.go:132: //sys	CreateProcess(appName *int16,
commandLine *uint16, procSecurity *int16, threadSecurity *int16, inheritHandles
int16, creationFlags uint32, env *uint16, currentDir *uint16, startupInfo
*StartupInfo, outProcInfo *ProcessInformation)  (ok uint32, errno int)
[failretval=0] = CreateProcessW
inheritHandles is not int16, it is 4 bytes long, you will get away with it here,
because in zsyscall_windows_386.go it is converted to uintptr anyway, but if you
do it in a struc, you will get burned.

If you make it inheritHandles bool, this:

diff -r a9f764657223 src/pkg/syscall/mksyscall_windows.sh
--- a/src/pkg/syscall/mksyscall_windows.sh      Thu Jun 10 13:30:39 2010 -0700
+++ b/src/pkg/syscall/mksyscall_windows.sh      Fri Jun 11 12:54:47 2010 +1000
@@ -133,6 +133,11 @@
                        } else {
                                push @args, "uintptr($name)", "uintptr($name >>
32)";
                        }
+               } elsif($type eq "bool") {
+                       $text .= "\tvar _p$n uint32;\n";
+                       $text .= "\tif $name { _p$n = 1; } else { _p$n = 0;
}\n";
+                       push @args, "uintptr(unsafe.Pointer(_p$n))";
                } else {
                        push @args, "uintptr($name)";
                }

should take care of it. Then you don't need to think twice what you must pass to
have your handles inherited - "0" or "1" or "-1" or "9999", it will be "true".

http://codereview.appspot.com/1578041/diff/11018/26006#newcode132
src/pkg/syscall/syscall_windows.go:132: //sys	CreateProcess(appName *int16,
commandLine *uint16, procSecurity *int16, threadSecurity *int16, inheritHandles
int16, creationFlags uint32, env *uint16, currentDir *uint16, startupInfo
*StartupInfo, outProcInfo *ProcessInformation)  (ok uint32, errno int)
[failretval=0] = CreateProcessW
Return values should be:

(ok bool, errno int) = CreateProcessW

like all other similar functions.

http://codereview.appspot.com/1578041/diff/11018/26008
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/1578041/diff/11018/26008#newcode175
src/pkg/syscall/ztypes_windows_386.go:175: _             uint8
It is *byte (4 bytes long), not uint8 (1 byte long).
Bug like that would be difficult to find - offsets of StdInput, StdOutput,
Stderr are now wrong and windows will not find you parameters there.

[kardia, 2010-06-14 01:34:46]

Made changes and modified for recent syscall change.  Tested, appears to
function.  Thank you for your patience.

http://codereview.appspot.com/1578041/diff/11018/26004
File src/pkg/syscall/exec_windows.go (right):

http://codereview.appspot.com/1578041/diff/11018/26004#newcode109
src/pkg/syscall/exec_windows.go:109: // 0x00000400 = UNICODE env
On 2010/06/11 11:16:37, brainman wrote:
> Do not need the comment about UNICODE now.
Removed

http://codereview.appspot.com/1578041/diff/11018/26006
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1578041/diff/11018/26006#newcode132
src/pkg/syscall/syscall_windows.go:132: //sys	CreateProcess(appName *int16,
commandLine *uint16, procSecurity *int16, threadSecurity *int16, inheritHandles
int16, creationFlags uint32, env *uint16, currentDir *uint16, startupInfo
*StartupInfo, outProcInfo *ProcessInformation)  (ok uint32, errno int)
[failretval=0] = CreateProcessW
Nice.  Had to remove unsafe.Pointer, as it is a value.

http://codereview.appspot.com/1578041/diff/11018/26006#newcode132
src/pkg/syscall/syscall_windows.go:132: //sys	CreateProcess(appName *int16,
commandLine *uint16, procSecurity *int16, threadSecurity *int16, inheritHandles
int16, creationFlags uint32, env *uint16, currentDir *uint16, startupInfo
*StartupInfo, outProcInfo *ProcessInformation)  (ok uint32, errno int)
[failretval=0] = CreateProcessW
On 2010/06/11 11:16:37, brainman wrote:
> Return values should be:
> 
> (ok bool, errno int) = CreateProcessW
> 
> like all other similar functions.
Done.

http://codereview.appspot.com/1578041/diff/11018/26008
File src/pkg/syscall/ztypes_windows_386.go (right):

http://codereview.appspot.com/1578041/diff/11018/26008#newcode175
src/pkg/syscall/ztypes_windows_386.go:175: _             uint8
On 2010/06/11 11:16:37, brainman wrote:
> It is *byte (4 bytes long), not uint8 (1 byte long).
> Bug like that would be difficult to find - offsets of StdInput, StdOutput,
> Stderr are now wrong and windows will not find you parameters there.
There are time I feel silly.  This is one of them.
Done.

[kardia, 2010-06-14 01:35:21]

Hello brainman, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-06-15 01:35:09]

http://codereview.appspot.com/1578041/diff/34001/35008
File src/pkg/syscall/syscall_windows.go (right):

http://codereview.appspot.com/1578041/diff/34001/35008#newcode132
src/pkg/syscall/syscall_windows.go:132: //sys	CreateProcess(appName *int16,
commandLine *uint16, procSecurity *int16, threadSecurity *int16, inheritHandles
bool, creationFlags uint32, env *uint16, currentDir *uint16, startupInfo
*StartupInfo, outProcInfo *ProcessInformation)  (ok bool, errno int)
[failretval=0] = CreateProcessW
Return values should be:

(ok bool, errno int) = CreateProcessW

You do not need [failretval=0] here. failretval is 0 by default, see
mksyscall_windows.sh. And having return value of type "bool" fits that notion
well.

[kardia, 2010-06-15 01:54:33]

Hello brainman, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[brainman, 2010-06-15 01:56:40]

LGTM

[kardia, 2010-06-30 20:55:58]

Hello brainman, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[kardia, 2010-06-30 21:02:34]

Hello brainman, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[kardia, 2010-06-30 21:03:30]

On 2010/06/30 21:02:34, kardia wrote:
> Hello brainman, rsc (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Updated patch set to merge with tip.

[alex.brainman_eventree.com, 2010-07-12 23:42:37]

Daniel,


> Updated patch set to merge with tip.
>

It's out of date again.

Now that Russ is back, would you like to refresh your change and will
get it moving.

Thank you.


Alex

[kardia, 2010-07-13 00:38:08]

Merged again.

Thanks,
-Daniel

[brainman, 2010-07-13 02:41:43]

On 2010/06/07 20:01:07, rsc1 wrote:
> ...

Ping.

[kardia, 2010-07-19 14:32:24]

I know people are busy but is there interest or comments to this?
Thanks,
-Daniel


On 2010/07/13 02:41:43, brainman wrote:
> On 2010/06/07 20:01:07, rsc1 wrote:
> > ...
> 
> Ping.

[brainman, 2010-07-19 23:43:05]

On 2010/07/19 14:32:24, kardia wrote:
> I know people are busy but is there interest or comments to this?

I can't submit your patch. But anyway you look at it, we need to implement
ForkExec(), even so we can just make goinstall to work. So, I suggest, you
continue, if you like. Get your goal to make goinstall work as intended, and do
whatever required.

Thank you.


Alex

[rsc, 2010-07-20 06:09:29]

Thanks very much for working on this.

This looks good to me but I'd like to see the environment
and fd slices addressed before we submit it.  The traceme
flag doesn't matter as much - it's just for the debugging library,
which isn't much used even on Linux.

The environment should be an argument to CreateProcess,
and the fd slice can turn into the three arguments passed
in the STARTUPINFO.  It's okay to return an error when
len(fd) > 3.

Russ

[kardia, 2010-07-22 00:21:50]

Russ, I wans't sure about the exact desired behavior of ForkExec.  I'v e made
the following comment:

//Behavior:
//	If envv is passed nil, then it uses the parent env
//	If a fd is nil, contains value <= 0, or not present, then it uses the parent
StdIn/Out/Err

However, I'm not sure if this matches up with how it behaves on unix.  Please
advise.  Thanks,
-Daniel


On 2010/07/20 06:09:29, rsc wrote:
> Thanks very much for working on this.
> 
> This looks good to me but I'd like to see the environment
> and fd slices addressed before we submit it.  The traceme
> flag doesn't matter as much - it's just for the debugging library,
> which isn't much used even on Linux.
> 
> The environment should be an argument to CreateProcess,
> and the fd slice can turn into the three arguments passed
> in the STARTUPINFO.  It's okay to return an error when
> len(fd) > 3.
> 
> Russ
>

[iant2, 2010-07-22 08:59:07]

kardianos@gmail.com writes:

> Russ, I wans't sure about the exact desired behavior of ForkExec.  I'v e
> made the following comment:
>
> //Behavior:
> //	If envv is passed nil, then it uses the parent env
> //	If a fd is nil, contains value <= 0, or not present, then it uses the
> parent StdIn/Out/Err
>
> However, I'm not sure if this matches up with how it behaves on unix.

On Unix, the environment passed to ForkExec is the environment that the
child process gets.  If you want the child to get the same environment
sa the parent, pass in os.Environ().

The fd array is the set of file descriptors passed to the child.  If the
array is nil the child starts with no open file descriptors.  If an
entry in the array is less than 0, that file descriptor is not open when
the child starts.  So, again, the child does not inherit from the
parent by default.

Ian

[kardia, 2010-07-22 15:33:48]

Please take another look.
Thanks,
-Daniel

[brainman, 2010-07-23 06:48:14]

I don't know much about this process. I think the only way we'll find out is by
testing it. LGTM.

Just one thing, the fds you pass to new process aren't inheritable. All current
apis create fds with non-inheritable flag (by default). So, again, we'll have to
test all that.


Alex

[rsc, 2010-07-23 06:56:26]

LGTM

leaving for alex

[kardia, 2010-07-23 14:23:26]

On a side note, I have tested the functionality and the fds and envv 
vars appear to function in the way described by the unix version.   I 
have to give startup info to each stdin/out/err so that if nothing is 
passed, then it doesn't get a valid stdout.  It's rather ironic, in 
order to not inherit fds, you have to inherit fds :) .

Thanks,
-Daniel


On 07/22/2010 11:48 PM, alex.brainman@gmail.com wrote:
> I don't know much about this process. I think the only way we'll find
> out is by testing it. LGTM.
>
> Just one thing, the fds you pass to new process aren't inheritable. All
> current apis create fds with non-inheritable flag (by default). So,
> again, we'll have to test all that.
>
>
> Alex
>
> http://codereview.appspot.com/1578041/show

[brainman, 2010-07-25 23:43:44]

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

syscall: add ForkExec, Syscall12 on Windows

R=brainman, rsc
CC=golang-dev
http://codereview.appspot.com/1578041

Committer: Alex Brainman <alex.brainman@gmail.com>