code review 1910041: fixes exec.LookPath, syscall.ForkExec on windows

[vcc, 2010-07-29 14:57:11]

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

I'd like you to review this change.

[kardia, 2010-07-30 04:57:49]

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

http://codereview.appspot.com/1910041/diff/2001/3002#newcode120
src/pkg/syscall/exec_windows.go:120: //Thus, repeating the exec name in the
first argument is unneeded.
Remove this comment to match new logic

[kardia, 2010-07-30 05:30:22]

On 2010/07/30 04:57:49, kardia wrote:
> http://codereview.appspot.com/1910041/diff/2001/3002
> File src/pkg/syscall/exec_windows.go (right):
> 
> http://codereview.appspot.com/1910041/diff/2001/3002#newcode120
> src/pkg/syscall/exec_windows.go:120: //Thus, repeating the exec name in the
> first argument is unneeded.
> Remove this comment to match new logic

Works for me.

[kardia, 2010-07-30 05:30:46]

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

http://codereview.appspot.com/1910041/diff/2001/3002#newcode165
src/pkg/syscall/exec_windows.go:165: StringToUTF16Ptr(escapeAddQuotes(argv0)+"
"+stringJoin(argv[1:], " ", escapeAddQuotes)),
Sorry for the two msgs.  Please check if len(argv) > 0 before you do this.

[brainman, 2010-08-01 08:28:57]

Thank you for the code, but I run pkg/exec test and it fails:

G:\>cd %GOROOT%\src\pkg\exec

G:\src\pkg\exec>8.out.exe
--- FAIL: exec.TestRunCat
        run: fork/exec /bin/cat: The system cannot find the path specified.
--- FAIL: exec.TestRunEcho
        run: fork/exec /bin/echo: The system cannot find the path specified.
--- FAIL: exec.TestStderr
        run: fork/exec /bin/sh: The system cannot find the path specified.
--- FAIL: exec.TestMergeWithStdout
        run: fork/exec /bin/sh: The system cannot find the path specified.
FAIL

G:\src\pkg\exec>

I'm not sure what are you attempting to solve here. I think the best way to move
forward here is to make goinstall work. This way we'll be able to discover what
really needs to be done in exec and os packages.

[vcc, 2010-08-01 11:45:25]

Make goinstall download work.

If change goinstall's domake to use make instead of gomake, it should also work.

For /bin/cat, /bin/sh, /bin/make, ... for test, I use cygwin, Just do like this:
cd \
junction bin c:\cygwin\bin

For junction: <http://technet.microsoft.com/en-us/sysinternals/bb896768.aspx>

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

http://codereview.appspot.com/1910041/diff/2001/3002#newcode120
src/pkg/syscall/exec_windows.go:120: //Thus, repeating the exec name in the
first argument is unneeded.
On 2010/07/30 04:57:49, kardia wrote:
> Remove this comment to match new logic

Done.

http://codereview.appspot.com/1910041/diff/2001/3002#newcode165
src/pkg/syscall/exec_windows.go:165: StringToUTF16Ptr(escapeAddQuotes(argv0)+"
"+stringJoin(argv[1:], " ", escapeAddQuotes)),
On 2010/07/30 05:30:46, kardia wrote:
> Sorry for the two msgs.  Please check if len(argv) > 0 before you do this.

Done.

[kardia, 2010-08-01 23:17:37]

I tested the exec logic, it does provide the additional modifications 
needed to get goinstall working.  Although I'm a little bit hesitant 
about not using DuplicateHandle(), as that is how I saw it done on msdn, 
it does appear to work.

I agree with Alex, it does not fix the exec_test, as I find needing to 
add a symlink to a cygwin env good for a long term solution, it does fix 
that ExecFork expects the first argument to to be the cmd name, and 
exec.LookPath now functions on Windows.  These two items fix a portion 
of goinstall (I'm still getting a hang on the build) but it's syncing 
correctly from the version control.

Would suggest renaming patch to something like "fixes exec.LookPath, 
syscall.ForkExec on windows".

[brainman, 2010-08-02 02:24:13]

It's much better, but, still, I'm not sure, if we're doing things correctly. 

I run test in pkg/exec and:

$ 8.out.exe
--- FAIL: exec.TestRunCat
        read: got ""
--- FAIL: exec.TestRunEcho
        run: fork/exec /bin/echo: The system cannot find the path specified.
--- FAIL: exec.TestStderr
        run: fork/exec /bin/sh: The system cannot find the path specified.
--- FAIL: exec.TestMergeWithStdout
        run: fork/exec /bin/sh: The system cannot find the path specified.
FAIL

If I change exec_test.go slightly:

diff -r f207af8c2d29 src/pkg/exec/exec_test.go
--- a/src/pkg/exec/exec_test.go	Mon Aug 02 09:52:15 2010 +1000
+++ b/src/pkg/exec/exec_test.go	Mon Aug 02 12:07:36 2010 +1000
@@ -11,7 +11,7 @@
 )
 
 func TestRunCat(t *testing.T) {
-	cmd, err := Run("/bin/cat", []string{"cat"}, nil, "",
+	cmd, err := Run("cat", []string{"cat"}, nil, "",
 		Pipe, Pipe, DevNull)
 	if err != nil {
 		t.Fatal("run:", err)

it finds cat command, but process running "cat" crashes with:

Application exception occurred:
        App:  (pid=2300)
        When: 02/08/2010 @ 12:02:56.765
        Exception number: c0000005 (access violation)

http://codereview.appspot.com/1910041/diff/14001/15005
File src/pkg/syscall/exec_windows.go (left):

http://codereview.appspot.com/1910041/diff/14001/15005#oldcode150
src/pkg/syscall/exec_windows.go:150: if ok, err := DuplicateHandle(currentProc,
int32(fd[0]), currentProc, &startupInfo.StdInput, 0, true,
DUPLICATE_SAME_ACCESS); !ok {
I'm not an expert, but I agree with Daniel. I'm not convinced you could just
pass the handle you hold to your child process.

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

http://codereview.appspot.com/1910041/diff/14001/15006#newcode681
src/pkg/syscall/syscall_windows.go:681: *wstatus = *(*WaitStatus)(&e)
This does not look right to me.

[vcc, 2010-08-02 07:44:01]

If use cat from msys, you can't run test under msys shell, cat will crash.
Run 8.out.exe under dos shell is ok, I don't know why, it's look like msys
shell's problem.

For DuplicateHandle, we use CreatePipe create these handles just for pass it
to child process, and we never use these passed handles, so don't need
duplicate it.
And the before code use DuplicateHandle is not work for me, it hang on test.

2010/8/2 <alex.brainman@gmail.com>

> It's much better, but, still, I'm not sure, if we're doing things
> correctly.
>
> I run test in pkg/exec and:
>
> $ 8.out.exe
> --- FAIL: exec.TestRunCat
>        read: got ""
>
> --- FAIL: exec.TestRunEcho
>        run: fork/exec /bin/echo: The system cannot find the path
> specified.
> --- FAIL: exec.TestStderr
>        run: fork/exec /bin/sh: The system cannot find the path
> specified.
> --- FAIL: exec.TestMergeWithStdout
>        run: fork/exec /bin/sh: The system cannot find the path
> specified.
> FAIL
>
> If I change exec_test.go slightly:
>
> diff -r f207af8c2d29 src/pkg/exec/exec_test.go
> --- a/src/pkg/exec/exec_test.go Mon Aug 02 09:52:15 2010 +1000
> +++ b/src/pkg/exec/exec_test.go Mon Aug 02 12:07:36 2010 +1000
> @@ -11,7 +11,7 @@
>  )
>
>  func TestRunCat(t *testing.T) {
> -       cmd, err := Run("/bin/cat", []string{"cat"}, nil, "",
> +       cmd, err := Run("cat", []string{"cat"}, nil, "",
>                Pipe, Pipe, DevNull)
>        if err != nil {
>                t.Fatal("run:", err)
>
> it finds cat command, but process running "cat" crashes with:
>
> Application exception occurred:
>        App:  (pid=2300)
>        When: 02/08/2010 @ 12:02:56.765
>        Exception number: c0000005 (access violation)
>
>
>
>
> http://codereview.appspot.com/1910041/diff/14001/15005
> File src/pkg/syscall/exec_windows.go (left):
>
> http://codereview.appspot.com/1910041/diff/14001/15005#oldcode150
> src/pkg/syscall/exec_windows.go:150: if ok, err :=
> DuplicateHandle(currentProc, int32(fd[0]), currentProc,
> &startupInfo.StdInput, 0, true, DUPLICATE_SAME_ACCESS); !ok {
> I'm not an expert, but I agree with Daniel. I'm not convinced you could
> just pass the handle you hold to your child process.
>
> http://codereview.appspot.com/1910041/diff/14001/15006
> File src/pkg/syscall/syscall_windows.go (right):
>
> http://codereview.appspot.com/1910041/diff/14001/15006#newcode681
> src/pkg/syscall/syscall_windows.go:681: *wstatus = *(*WaitStatus)(&e)
> This does not look right to me.
>
>
> http://codereview.appspot.com/1910041/show
>

[vcc, 2010-08-02 12:46:48]

On 2010/08/01 23:17:37, kardia wrote:
> Would suggest renaming patch to something like "fixes exec.LookPath, 
> syscall.ForkExec on windows".

Done, thanks.

[vcc, 2010-08-02 12:55:53]

Add get exit code and change to use DuplicateHandle to support use file as
STDIN/OUT/ERR, please take another look.

I just think about should support check bash script in LookPath to exec shell
script?

http://codereview.appspot.com/1910041/diff/14001/15005
File src/pkg/syscall/exec_windows.go (left):

http://codereview.appspot.com/1910041/diff/14001/15005#oldcode150
src/pkg/syscall/exec_windows.go:150: if ok, err := DuplicateHandle(currentProc,
int32(fd[0]), currentProc, &startupInfo.StdInput, 0, true,
DUPLICATE_SAME_ACCESS); !ok {
On 2010/08/02 02:24:13, brainman wrote:
> I'm not an expert, but I agree with Daniel. I'm not convinced you could just
> pass the handle you hold to your child process.

Done.

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

http://codereview.appspot.com/1910041/diff/14001/15006#newcode681
src/pkg/syscall/syscall_windows.go:681: *wstatus = *(*WaitStatus)(&e)
On 2010/08/02 02:24:13, brainman wrote:
> This does not look right to me.

Done.

[vcc, 2010-08-02 16:26:16]

goinstall work now!
just create gomake.bat in $GOROOT/bin, content as follow:
make %1 %2 %3 %4 %5 %6 %7 %8 %9

That everything is work!

Wei gaungjing

[rsc1, 2010-08-26 16:49:58]

vcc, brainman: what's the status of this CL?

[Joe Poirier, 2010-08-26 17:59:59]

On Thu, Aug 26, 2010 at 11:49 AM,  <rsc@google.com> wrote:
> vcc, brainman: what's the status of this CL?
>
>
> http://codereview.appspot.com/1910041/
>

I apologize for being late to the show!

I missed this CL and neglected to search for any existing LookPath
CLs when I posted my comments about moving it to the os package.

It may be better to split this cl in to two parts, one for LookPath
and the other for ForkExec. Leave the ForkExec in this cl and
I can submit the cl for LookPath.

On a side note, the path separator handling for the windows version
needs should be enhanced.

-joe

[brainman, 2010-08-26 23:37:46]

> It may be better to split this cl in to two parts, one for LookPath
> and the other for ForkExec. 

I like the idea.

> ... Leave the ForkExec in this cl and
> I can submit the cl for LookPath.

If everyone agrees to move LookPath() to os, we could create new CL that
implements os.LookPath() working properly for all $GOOS. If anyone wants to do
it you're welcome, otherwise I'll get around to it myself.

> On a side note, the path separator handling for the windows version
> needs should be enhanced.

I didn't look at it yet, but, I suspect, that would be the case. Mind you, while
I agree it might be related to LookPath(), it, probably, should be dealt in a
separate CL again.


Alex

[brainman, 2010-08-26 23:39:07]

On 2010/08/26 16:49:58, rsc1 wrote:
> vcc, brainman: what's the status of this CL?

Waiting for http://code.google.com/p/go/issues/detail?id=1004 to be resolved
first.


Alex

[Joe Poirier, 2010-08-27 00:16:59]

> I like the idea.
>
>> ... Leave the ForkExec in this cl and
>> I can submit the cl for LookPath.

Okay, then the author of cl1910041 can remove the LookPath changes

> If everyone agrees to move LookPath() to os, we could create new CL that
> implements os.LookPath() working properly for all $GOOS. If anyone wants
> to do it you're welcome, otherwise I'll get around to it myself.

I assumed LookPath worked for all GOOSs /except/ windows, is that not
the case? If it is the case then the current LookPath and cmdExec code
gets moved to os/env_unix.go and modified (to run on windows) version
go to os/env_windows.go

>> On a side note, the path separator handling for the windows version
>> needs should be enhanced.
>
> I didn't look at it yet, but, I suspect, that would be the case. Mind
> you, while I agree it might be related to LookPath(), it, probably,
> should be dealt in a separate CL again.

For the windows mods, it's just a matter of switching from forward
to (escaped)backward slashes and possibly adding one or two conditional
checks for the odd scenario. It's easy enough to test (already played with
it a bit) and include the mods when I move LookPath to the os package.

-joe

[rsc, 2010-08-27 00:22:08]

>> If everyone agrees to move LookPath() to os, we could create new CL that
>> implements os.LookPath() working properly for all $GOOS. If anyone wants
>> to do it you're welcome, otherwise I'll get around to it myself.
>
> I assumed LookPath worked for all GOOSs /except/ windows, is that not
> the case? If it is the case then the current LookPath and cmdExec code
> gets moved to os/env_unix.go and modified (to run on windows) version
> go to os/env_windows.go

Sounds good.

Russ

[vcc, 2010-08-27 05:29:22]

Removed LookPath code, please take another look.

Wei guangjing

On 2010/08/27 00:22:08, rsc wrote:
> >> If everyone agrees to move LookPath() to os, we could create new CL that
> >> implements os.LookPath() working properly for all $GOOS. If anyone wants
> >> to do it you're welcome, otherwise I'll get around to it myself.
> >
> > I assumed LookPath worked for all GOOSs /except/ windows, is that not
> > the case? If it is the case then the current LookPath and cmdExec code
> > gets moved to os/env_unix.go and modified (to run on windows) version
> > go to os/env_windows.go
> 
> Sounds good.
> 
> Russ
>

[vcc, 2010-08-27 05:41:49]

On 2010/08/26 23:39:07, brainman wrote:
> On 2010/08/26 16:49:58, rsc1 wrote:
> > vcc, brainman: what's the status of this CL?
> 
> Waiting for http://code.google.com/p/go/issues/detail?id=1004 to be resolved
> first.
 
I suggest submit this CL first, since we can use OpenProcess get process handle
from pid, so make things work now and improve in the future is better.

Wei guangjing

[brainman, 2010-08-27 05:47:02]

On 2010/08/27 05:41:49, vcc wrote:

> I suggest submit this CL first, since we can use OpenProcess get process
handle
> from pid, so make things work now and improve in the future is better.

What do we do when OpenProcess fails with ERROR_INVALID_PARAMETER as it happened
to me?

I think it is best we wait for Joe and Russ make their changes, then your change
should be much simpler.


Alex

[vcc, 2010-08-27 09:19:28]

On 2010/08/27 05:47:02, brainman wrote:
> 
> What do we do when OpenProcess fails with ERROR_INVALID_PARAMETER as it
happened
> to me?

ERROR_INVALID_PARAMETER only happen when try to open a system process, open
process by us create should always ok.

And wait4 need support only have pid, since maybe need wait for a process got
pid from conf files, or got pid from other process, ....

Wei guangjing

[brainman, 2010-08-27 12:23:22]

On 2010/08/27 09:19:28, vcc wrote:

> ERROR_INVALID_PARAMETER only happen when try to open a system process, ...

see this story http://tinyurl.com/2ec82n9 (it has happened to me too)

see this question and 2 answers http://tinyurl.com/25um4dq

see some code in http://tinyurl.com/28w4d2d

> open
> process by us create should always ok.

It seems to me, that once target process has finished, OpenProcess() will fail.
Who are you going to ask about ExitCode (GetExitCodeProcess() needs process
handle, but if OpenProcess fails you're stuck)?

Better option is to keep handle returned by CreateProcess around and use it to
Wait...() and finally GetExitCodeProcess().


Alex

[vcc, 2010-08-27 15:44:53]

2010/8/27 <alex.brainman@gmail.com>

> On 2010/08/27 09:19:28, vcc wrote:
>
>  ERROR_INVALID_PARAMETER only happen when try to open a system process,
>>
> ...
>
> see this story http://tinyurl.com/2ec82n9 (it has happened to me too)
>
> see this question and 2 answers http://tinyurl.com/25um4dq
>
> see some code in http://tinyurl.com/28w4d2d
>
>
>  open
>> process by us create should always ok.
>>
>
> It seems to me, that once target process has finished, OpenProcess()
> will fail. Who are you going to ask about ExitCode (GetExitCodeProcess()
> needs process handle, but if OpenProcess fails you're stuck)?
>

I found OpenProcess never fail with child process, even the child process is
exited. So don't need worry about it.

Wei guangjing

[brainman, 2010-10-12 01:45:58]

On 2010/08/27 00:22:08, rsc wrote:

LGTM.

Russ,

I would like to move forward with this. I propose, we submit this change (after
Wei updates it to the current tip) as is. I feel that Wei and Joe have done
excellent job and we're nearly there with exec package and maybe goinstall.

Once submitted, I'll make more changes (I'll try to implement
http://code.google.com/p/go/issues/detail?id=1004 and some other changes) and
I'll clean up all exec related source code.

Alex

[rsc, 2010-10-12 01:53:11]

LGTM

Sorry, I didn't realize you were waiting on me.

[brainman, 2010-10-12 01:59:46]

On 2010/10/12 01:53:11, rsc wrote:
> LGTM
> 
> Sorry, I didn't realize you were waiting on me.

I was hoping you'll do http://code.google.com/p/go/issues/detail?id=1004 <g>,
but, never mind, I'll have a go myself. If you have any suggestions, do tell.

To Wei, please, update your code to the tip and I'll submit it. Thank you.

Alex

[vcc, 2010-10-12 02:57:25]

Thanks for review, please take another look.

[brainman, 2010-10-12 04:42:13]

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

syscall: implement WaitStatus and Wait4() for windows

R=brainman, rsc, kardia, Joe Poirier
CC=golang-dev
http://codereview.appspot.com/1910041

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