code review 4253052: os, syscall: add ProcAttributes type. Change StartProce...

os, syscall: add ProcAttr type. Change StartProcess etc. to use it.
The Windows code is untested.

[rog, 2011-03-03 20:04:03]

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

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

[rsc, 2011-03-03 20:07:04]

s/Attributes/Attr/

why not put the fd and env there too?

[rog, 2011-03-03 20:07:54]

one possibility for the ProcAttributes methods is
to have them return the receiver so that they can
be chained.

e.g.

StartProcess(.... new(os.ProcAttributes).Chdir(dir))

instead of:

var attrs os.ProcAttributes
attrs.Chdir(dir)
StartProcess(... &attrs)

i can't decide if that would be a good thing, or too Un-Go.


On 3 March 2011 20:04,  <rogpeppe@gmail.com> wrote:
> Reviewers: rsc, gri,
>
> Message:
> Hello rsc, gri (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> os, syscall: add ProcAttributes type. Change StartProcess etc. to use
> it.
> The Windows code is untested.
>
> Please review this at http://codereview.appspot.com/4253052/
>
> Affected files:
>  M src/cmd/cgo/util.go
>  M src/cmd/godoc/main.go
>  M src/pkg/exec/exec.go
>  M src/pkg/http/triv.go
>  M src/pkg/os/exec.go
>  M src/pkg/os/os_test.go
>  M src/pkg/syscall/exec_unix.go
>  M src/pkg/syscall/exec_windows.go
>
>
>

[rog, 2011-03-03 20:08:18]

why not indeed.

On 3 March 2011 20:07, Russ Cox <rsc@golang.org> wrote:
> s/Attributes/Attr/
>
> why not put the fd and env there too?
>

[rsc, 2011-03-03 20:09:00]

On Thu, Mar 3, 2011 at 15:07, roger peppe <rogpeppe@gmail.com> wrote:
> one possibility for the ProcAttributes methods is
> to have them return the receiver so that they can
> be chained.

please don't.

[rsc, 2011-03-03 20:16:47]

The reason for the helpers is not obvious to me.
Why can't we just define a struct with some public
fields and let the caller fill it in however it likes?

Russ

[rog, 2011-03-03 20:30:13]

On 3 March 2011 20:10, Russ Cox <rsc@golang.org> wrote:
> The reason for the helpers is not obvious to me.
> Why can't we just define a struct with some public
> fields and let the caller fill it in however it likes?

a few reasons, none of which are necessarily compelling,
but were collectively enough for me.

- with the helper functions you can convert *syscall.ProcAttr
to *os.ProcAttr if you want, which means that you don't
have to change your code to use syscall.StartProcess the
moment you want a system-specific process attribute.
the alternative is to export system-specific fields in os.ProcAttr
which wouldn't be good.

- i wanted the possibility to represent more involved attributes,
such as you might find inside a unix fork-exec sequence,
without complicating the interface. my motivating thought
was sequences of dups and closes; maybe there's nothing else.

- with the helper functions it's easy to have different
underlying representations if that's convenient for a particular
platform.

mind you, none of this means that we can't have some public
fields, e.g. Dir.

[brainman, 2011-03-04 00:02:16]

http://codereview.appspot.com/4253052/diff/2002/src/pkg/os/exec.go
File src/pkg/os/exec.go (left):

http://codereview.appspot.com/4253052/diff/2002/src/pkg/os/exec.go#oldcode29
src/pkg/os/exec.go:29: // If dir is not empty, the child chdirs into the
directory before execing the program.
Should get rid of mention of dir in this comment.

http://codereview.appspot.com/4253052/diff/2002/src/pkg/syscall/exec_windows.go
File src/pkg/syscall/exec_windows.go (left):

http://codereview.appspot.com/4253052/diff/2002/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:129: dir = wd
It doesn't compile:

rex syscall # make install
8g -o _go_.8 str.go syscall.go syscall_386.go syscall_windows.go
syscall_windows_386.go ze
rrors_windows_386.go zsyscall_windows_386.go zsysnum_windows_386.go
ztypes_windows_386.go 
 exec_windows.go
exec_windows.go:141: undefined: dir
exec_windows.go:141: cannot assign to dir
make: *** [_go_.8] Error 1

Perhaps this will do:

diff -r 94d654be2064 src/pkg/syscall/exec_windows.go
--- a/src/pkg/syscall/exec_windows.go	Thu Mar 03 13:29:31 2011 -0800
+++ b/src/pkg/syscall/exec_windows.go	Fri Mar 04 10:32:14 2011 +1100
@@ -114,16 +114,27 @@
 	return 0
 }
 
+type ProcAttributes struct {
+	dir string
+}
+
+func (a *ProcAttributes) Chdir(dir string) {
+	a.dir = dir
+}
 
 // TODO(kardia): Add trace
 //The command and arguments are passed via the Command line parameter.
-func StartProcess(argv0 string, argv []string, envv []string, dir string, fd
[]int) (pid, handle int, err int) {
+func StartProcess(argv0 string, argv []string, envv []string, fd []int, attrs
*ProcAttributes) (pid, handle int, err int) {
 	if len(fd) > 3 {
 		return 0, 0, EWINDOWS
 	}
+	var dir string
+	if attrs != nil {
+		dir = attrs.dir
+	}
 
-	//CreateProcess will throw an error if the dir is not set to a valid dir
-	//  thus get the working dir if dir is empty.
+	// CreateProcess will throw an error if the dir is not set to a valid dir
+	// thus get the working dir if dir is empty.
 	if len(dir) == 0 {
 		if wd, ok := Getwd(); ok == 0 {
 			dir = wd

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

http://codereview.appspot.com/4253052/diff/2002/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:139: if attrs.dir != "" {
You reversed condition here. I think it is mistake.

[rog, 2011-03-04 09:32:43]

On 3 March 2011 20:30, roger peppe <rogpeppe@gmail.com> wrote:
> - with the helper functions you can convert *syscall.ProcAttr
> to *os.ProcAttr if you want, which means that you don't
> have to change your code to use syscall.StartProcess the
> moment you want a system-specific process attribute.

this is the crux of the issue. if you can do this, then you
can still use helper packages to start your processes
for you, for example the exec package, while keeping
the ability to set system-specific attributes on the
new process (for instance blocked signals).

if we don't care about that, then i'll go with a simple struct
type for both syscall and os, and some code to translate
from os.ProcAttr to syscall.ProcAttr.

thoughts?

[rsc, 2011-03-05 20:19:23]

I think syscall and os should have different structures.
os.StartProcess will have to create a syscall version of
the structure from the os one.

[rog, 2011-03-08 12:14:05]

On 2011/03/05 20:19:23, rsc wrote:
> I think syscall and os should have different structures.
> os.StartProcess will have to create a syscall version of
> the structure from the os one.

yes. i was trying to be too clever - wanted to be able to do things like setuid
while still using the os interface.

i'm trying to upload a new version, but i keep on getting errors and occasional
stack traces from the codereview extension:

Loading /4253052/edit: urllib2.HTTPError: HTTP Error 500: Internal Server Error;
trying again in 2 seconds.
abort: HTTP Error 500: Internal Server Error

the site appears to be up, so perhaps i have mucked up local state.

[rog, 2011-03-08 14:40:30]

upload succeeded eventually (maybe something timed out on the server).

PTAL

[rsc, 2011-03-08 17:43:28]

make sure that

GOARCH=386 GOOS=windows make

works in package syscall

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

http://codereview.appspot.com/4253052/diff/7010/src/pkg/os/exec.go#newcode27
src/pkg/os/exec.go:27: // If Dir is non-empty, the child chdirs into the
directory before
s/chdir into/changes to/

http://codereview.appspot.com/4253052/diff/7010/src/pkg/os/exec.go#newcode28
src/pkg/os/exec.go:28: // creating the process.
The program name passed to StartProcess is 
interpreted after changing the directory.

http://codereview.appspot.com/4253052/diff/7010/src/pkg/os/exec.go#newcode34
src/pkg/os/exec.go:34: // Files specifies the file descriptors to be set up in
the new process:
// Files specifies the open files inherited by the new process.
// The first three entries correspond to standard input, standard output,
// and standard error.  An implementation may support additional entries,
// depending on the underlying operating system.
// A nil entry corresponds to that file being closed when the process starts.

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

http://codereview.appspot.com/4253052/diff/7010/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:117: type ProcAttributes struct {
ProcAttr please

[brainman, 2011-03-08 23:02:09]

On 2011/03/08 14:40:30, rog wrote:
> PTAL

GOARCH=386 GOOS=linux fails with:

rex src # cd pkg/debug/proc/
rex proc # make clean install
rm -rf *.o *.a *.[568vq] [568vq].out *.so _obj _test _testmain.go *.exe _cgo*
*.cgo[12].*
8g -o _go_.8 proc.go proc_linux.go regs_linux_386.go
proc_linux.go:1300: undefined: syscall.PtraceForkExec
make: *** [_go_.8] Error 1
rex proc #


GOARCH=386 GOOS=windows fails with:

rex src # cd pkg/syscall/
rex syscall # make clean install
rm -rf *.o *.a *.[568vq] [568vq].out *.so _obj _test _testmain.go *.exe _cgo*
*.cgo[12].*
8g -o _go_.8 str.go syscall.go syscall_386.go syscall_windows.go
syscall_windows_386.go zerrors_windows_386.go zsyscall_windows_386.go
zsysnum_windows_386.go ztypes_windows_386.go exec_windows.go
exec_windows.go:127: undefined: ProcAttr
make: *** [_go_.8] Error 1
rex syscall #

[rog, 2011-03-09 16:19:59]

On 8 March 2011 17:43,  <rsc@golang.org> wrote:
> make sure that
>
> GOARCH=386 GOOS=windows make
>
> works in package syscall

done (a bit awkward though - i had
to manually rebuild its dependencies (sync, sync/atomic, runtime,
unicode and utf16)
before it would get past the import stage).
i wonder if an extra option to goinstall to give a list of
package dependencies would be useful.

i also tried to get Go building under windows mingw,
which i succeeded in doing (with the exception of the
compilers) but then i couldn't work out how to get the
codereview extension working, which needs the HtmlParser module,
which isn't included with the python bundled with the mercurial install.
too much time required!

[rog, 2011-03-09 16:20:36]

PTAL

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

http://codereview.appspot.com/4253052/diff/7010/src/pkg/os/exec.go#newcode27
src/pkg/os/exec.go:27: // If Dir is non-empty, the child chdirs into the
directory before
On 2011/03/08 17:43:28, rsc wrote:
> s/chdir into/changes to/
> 

Done.

http://codereview.appspot.com/4253052/diff/7010/src/pkg/os/exec.go#newcode28
src/pkg/os/exec.go:28: // creating the process.
On 2011/03/08 17:43:28, rsc wrote:
> The program name passed to StartProcess is 
> interpreted after changing the directory.

that's not true of windows. i'm not sure of the best way to resolve that.

http://codereview.appspot.com/4253052/diff/7010/src/pkg/os/exec.go#newcode34
src/pkg/os/exec.go:34: // Files specifies the file descriptors to be set up in
the new process:
On 2011/03/08 17:43:28, rsc wrote:
> // Files specifies the open files inherited by the new process.
> // The first three entries correspond to standard input, standard output,
> // and standard error.  An implementation may support additional entries,
> // depending on the underlying operating system.
> // A nil entry corresponds to that file being closed when the process starts.
> 

Done.

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

http://codereview.appspot.com/4253052/diff/7010/src/pkg/syscall/exec_windows....
src/pkg/syscall/exec_windows.go:117: type ProcAttributes struct {
On 2011/03/08 17:43:28, rsc wrote:
> ProcAttr please

Done.

[rog, 2011-03-09 16:21:33]

On 8 March 2011 23:02,  <alex.brainman@gmail.com> wrote:
> proc_linux.go:1300: undefined: syscall.PtraceForkExec

i've modified this file, but i have not got a linux box, so have not
been able to test it.

[brainman, 2011-03-10 04:58:32]

GOARCH=386 GOOS=linux still fails with:

rex src # cd pkg/debug/proc/                                                    
         
rex proc # make clean install
rm -rf *.o *.a *.[568vq] [568vq].out *.so _obj _test _testmain.go *.exe _cgo*
*.cgo[12].* 8g -o _go_.8 proc.go proc_linux.go regs_linux_386.go                
                     
proc_linux.go:1294: undefined: fd
proc_linux.go:1295: undefined: fd
proc_linux.go:1296: typechecking loop
proc_linux.go:1297: typechecking loop
proc_linux.go:1306: assignment count mismatch: 2 = 3
make: *** [_go_.8] Error 1
rex proc #


GOARCH=386 GOOS=windows builds, but fails test in pkg/exec. Please see my
comment in pkg/syscall/syscall_windows.go.


I could be wrong, because I didn't test it, but this instructions might help you
build for different arch/os.

First build everything for your host environment:

cd $GOROOT/src
./make.bash

if I assume that you use GOARCH=amd64 and GOOS=darwin, then this will create all
6* build tools (6g, 6l and others) in $GOBIN and compile all go packages into
your $GOROOT/pkg/darwin_amd64/.

Next you should make build tools for other archs. For example, to create and
install all 8* tools you should be able to:

cd $GOROOT/src/cmd
export GOARCH=386
make clean install

clean will delete all temp files, install will build all progs install them into
$GOBIN alongside with 6*.

Next your want to build packages for other arch/os. For example, to build
386/linux you do:

cd $GOROOT/src/pkg
export GOARCH=386
export GOOS=linux
make clean deps install

clean will delete all temp files, deps will rebuild source file dependencies
(different arch/os use different source files), install will build packages and
install them into $GOROOT/pkg/linux_386/.

Windows pkg build needs one extra env variable for pkg build to proceed on my pc
(linux):

cd $GOROOT/src/pkg
export GOARCH=386
export GOOS=windows
export DISABLE_CGO=1
make clean deps install

Good luck.

Alex

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

http://codereview.appspot.com/4253052/diff/21010/src/pkg/syscall/exec_windows...
src/pkg/syscall/exec_windows.go:196: StringToUTF16Ptr(attr.Dir),
You have changed dir, not attr.Dir, you want to use dir in the call:
StringToUTF16Ptr(dir),

[rog, 2011-03-10 08:51:08]

On 10 March 2011 04:58,  <alex.brainman@gmail.com> wrote:
> GOARCH=386 GOOS=linux still fails with:
>
> rex src # cd pkg/debug/proc/
>
> rex proc # make clean install
> rm -rf *.o *.a *.[568vq] [568vq].out *.so _obj _test _testmain.go *.exe
> _cgo* *.cgo[12].* 8g -o _go_.8 proc.go proc_linux.go regs_linux_386.go
>
> proc_linux.go:1294: undefined: fd
> proc_linux.go:1295: undefined: fd
> proc_linux.go:1296: typechecking loop
> proc_linux.go:1297: typechecking loop
> proc_linux.go:1306: assignment count mismatch: 2 = 3
> make: *** [_go_.8] Error 1
> rex proc #
>
>
> GOARCH=386 GOOS=windows builds, but fails test in pkg/exec. Please see
> my comment in pkg/syscall/syscall_windows.go.
>
>
> I could be wrong, because I didn't test it, but this instructions might
> help you build for different arch/os.
>
> First build everything for your host environment:
>
> cd $GOROOT/src
> ./make.bash
>
> if I assume that you use GOARCH=amd64 and GOOS=darwin, then this will
> create all 6* build tools (6g, 6l and others) in $GOBIN and compile all
> go packages into your $GOROOT/pkg/darwin_amd64/.
>
> Next you should make build tools for other archs. For example, to create
> and install all 8* tools you should be able to:
>
> cd $GOROOT/src/cmd
> export GOARCH=386
> make clean install

i needed to do this first (sorry, rc syntax):
cd $GOROOT/src; for(i in libbio libcgo lib9 libmach){@{cd $i && make
clean install}}

> cd $GOROOT/src/pkg
> export GOARCH=386
> export GOOS=linux
> make clean deps install
>
> clean will delete all temp files, deps will rebuild source file
> dependencies (different arch/os use different source files), install
> will build packages and install them into $GOROOT/pkg/linux_386/.

i needed DISABLE_CGO for this too.

> Windows pkg build needs one extra env variable for pkg build to proceed
> on my pc (linux):
>
> cd $GOROOT/src/pkg
> export GOARCH=386
> export GOOS=windows
> export DISABLE_CGO=1
> make clean deps install
>
> Good luck.

thanks for that.
it all compiles now, at any rate, and that dir vs attr.Dir bug is fixed.

PTAL

[brainman, 2011-03-10 22:57:35]

LGTM

[rsc1, 2011-03-15 17:14:35]

Otherwise LGTM

http://codereview.appspot.com/4253052/diff/4019/src/pkg/syscall/exec_unix.go
File src/pkg/syscall/exec_unix.go (right):

http://codereview.appspot.com/4253052/diff/4019/src/pkg/syscall/exec_unix.go#...
src/pkg/syscall/exec_unix.go:106: // we could copy this if we cared about the
side effects, but this is syscall.
what side effects?

[rog, 2011-03-15 17:29:33]

http://codereview.appspot.com/4253052/diff/4019/src/pkg/syscall/exec_unix.go
File src/pkg/syscall/exec_unix.go (right):

http://codereview.appspot.com/4253052/diff/4019/src/pkg/syscall/exec_unix.go#...
src/pkg/syscall/exec_unix.go:106: // we could copy this if we cared about the
side effects, but this is syscall.
On 2011/03/15 17:14:35, rsc1 wrote:
> what side effects?

the fd array is assigned to when the file descriptors are juggled (see line 176
below). i presumed that was ok since that's how it was.

[rsc1, 2011-03-15 17:31:50]

Could you make a copy?  I don't think I did that intentionally.

Russ

[rog, 2011-03-15 17:37:19]

On 2011/03/15 17:31:50, rsc1 wrote:
> Could you make a copy?  I don't think I did that intentionally.
> 
> Russ

done

[rsc1, 2011-03-15 17:38:34]

LGTM

[rsc, 2011-03-15 18:41:50]

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

os, syscall: add ProcAttr type. Change StartProcess etc. to use it.
The Windows code is untested.

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

Committer: Russ Cox <rsc@golang.org>