code review 3816043: syscall: Plan 9 support for x86.

[paulzhol, 2010-12-26 15:19:22]

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

I'd like you to review this change.

[brainman, 2010-12-30 01:29:32]

http://codereview.appspot.com/3816043/diff/9001/src/pkg/os/env_plan9.go
File src/pkg/os/env_plan9.go (right):

http://codereview.appspot.com/3816043/diff/9001/src/pkg/os/env_plan9.go#newco...
src/pkg/os/env_plan9.go:131: }
You said "runtime support for getenv is missing", if you mean that os.Env is not
initialized, then just add this:

func init() {
  Envs = Environ()
}

os.Args should already be set by runtime.

http://codereview.appspot.com/3816043/diff/9001/src/pkg/os/file_posix.go
File src/pkg/os/file_posix.go (right):

http://codereview.appspot.com/3816043/diff/9001/src/pkg/os/file_posix.go#newc...
src/pkg/os/file_posix.go:7: package os
You should do "hg cp ..." instead of "hg add ..." for that file (and others), to
retain change history.

http://codereview.appspot.com/3816043/diff/9001/src/pkg/syscall/asm_plan9_386.s
File src/pkg/syscall/asm_plan9_386.s (right):

http://codereview.appspot.com/3816043/diff/9001/src/pkg/syscall/asm_plan9_386...
src/pkg/syscall/asm_plan9_386.s:6: // System call support for 386, Plan 9
I'm not good with asm, so take all my comments with grain of salt.

http://codereview.appspot.com/3816043/diff/9001/src/pkg/syscall/asm_plan9_386...
src/pkg/syscall/asm_plan9_386.s:18: // slide args down on top of system call
number
Just an optimization: you could achieve the same by just adjusting SP by 4 and
moving return address one word up. Same for Syscall6.

http://codereview.appspot.com/3816043/diff/9001/src/pkg/syscall/asm_plan9_386...
src/pkg/syscall/asm_plan9_386.s:68: // call errstr with buffer on the stack
I would be careful here - you're running on "go stack". You can't assume there
is infinite amount of space on stack. In fact, this function, being (#pragma
textflag 7), is not checking if it needs to grow stack at the start, therefore
it becomes your responsibility.

There is too much happening here anyway, I would implement it in C or Go
instead: just pass ready to use buffer for the error string to your asm function
and parse it in C/Go once it returns.

http://codereview.appspot.com/3816043/diff/9001/src/pkg/syscall/syscall.go
File src/pkg/syscall/syscall.go (left):

http://codereview.appspot.com/3816043/diff/9001/src/pkg/syscall/syscall.go#ol...
src/pkg/syscall/syscall.go:16: func Syscall(trap, a1, a2, a3 uintptr) (r1, r2,
err uintptr)
I'm not sure, you should change signature of all these functions. You're not
calling these functions by hand, but generate all calls automagically - you
could ignore some return values, if they are not used.

If you insist on that change, please copy these lines into syscall_windows.go
the way you copied them into syscall_unix.go, otherwise windows build fails.

[paulzhol, 2010-12-31 13:11:13]

Thanks for the review and happy new year!

Pavel

http://codereview.appspot.com/3816043/diff/9001/src/pkg/os/env_plan9.go
File src/pkg/os/env_plan9.go (right):

http://codereview.appspot.com/3816043/diff/9001/src/pkg/os/env_plan9.go#newco...
src/pkg/os/env_plan9.go:131: }
I meant runtime·getenv is not doing the right thing for Plan 9. It goes over the
os.Envs slice which is always empty, instead of reading the filesystem provided
by a special device which is exporting the environment ('#e/').

I'm not sure adding os.init() is the right solution because runtime·getenv is
called very early, in runtime·schedinit for exmple.
I just didn't have the chance to code it in C for runtime yet.

On 2010/12/30 01:29:32, brainman wrote:
> You said "runtime support for getenv is missing", if you mean that os.Env is
not
> initialized, then just add this:
> 
> func init() {
>   Envs = Environ()
> }
> 
> os.Args should already be set by runtime.

http://codereview.appspot.com/3816043/diff/9001/src/pkg/os/file_posix.go
File src/pkg/os/file_posix.go (right):

http://codereview.appspot.com/3816043/diff/9001/src/pkg/os/file_posix.go#newc...
src/pkg/os/file_posix.go:7: package os
My mistake, will fix.

On 2010/12/30 01:29:32, brainman wrote:
> You should do "hg cp ..." instead of "hg add ..." for that file (and others),
to
> retain change history.

http://codereview.appspot.com/3816043/diff/9001/src/pkg/syscall/asm_plan9_386.s
File src/pkg/syscall/asm_plan9_386.s (right):

http://codereview.appspot.com/3816043/diff/9001/src/pkg/syscall/asm_plan9_386...
src/pkg/syscall/asm_plan9_386.s:18: // slide args down on top of system call
number
I think that in 8g the caller expects the results to be in the first location
right after the arguments which means they are first on the stack - so you can't
adjust the return address (and this is also the way FreeBSD is doing it). 

On 2010/12/30 01:29:32, brainman wrote:
> Just an optimization: you could achieve the same by just adjusting SP by 4 and
> moving return address one word up. Same for Syscall6.

http://codereview.appspot.com/3816043/diff/9001/src/pkg/syscall/asm_plan9_386...
src/pkg/syscall/asm_plan9_386.s:68: // call errstr with buffer on the stack
You are right about the segmented stacks, I totally forgot about them.

I had an extra slice argument I passed in earlier versions, but because I always
had to take the address of the slice with unsafe.Pointer, 8g would always
allocate it on the heap for each syscall.

I don't know if it is alright to call a function which can grow its stack from
inside one that is not doing the checks, but I'll move these bits into errstr()
in syscall_paln9.go.

On 2010/12/30 01:29:32, brainman wrote:
> I would be careful here - you're running on "go stack". You can't assume there
> is infinite amount of space on stack. In fact, this function, being (#pragma
> textflag 7), is not checking if it needs to grow stack at the start, therefore
> it becomes your responsibility.
> 
> There is too much happening here anyway, I would implement it in C or Go
> instead: just pass ready to use buffer for the error string to your asm
function
> and parse it in C/Go once it returns.

http://codereview.appspot.com/3816043/diff/9001/src/pkg/syscall/syscall.go
File src/pkg/syscall/syscall.go (left):

http://codereview.appspot.com/3816043/diff/9001/src/pkg/syscall/syscall.go#ol...
src/pkg/syscall/syscall.go:16: func Syscall(trap, a1, a2, a3 uintptr) (r1, r2,
err uintptr)
Oops, this was left over from one of the earlier attempts. I think I call
Syscall only once anyway. I'll revert it back.

On 2010/12/30 01:29:32, brainman wrote:
> I'm not sure, you should change signature of all these functions. You're not
> calling these functions by hand, but generate all calls automagically - you
> could ignore some return values, if they are not used.
> 
> If you insist on that change, please copy these lines into syscall_windows.go
> the way you copied them into syscall_unix.go, otherwise windows build fails.

[paulzhol, 2011-01-03 21:29:13]

Hi, please take another look.

Pavel

[brainman, 2011-01-03 23:12:19]

LGTM

http://codereview.appspot.com/3816043/diff/28001/src/pkg/syscall/syscall_plan...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/28001/src/pkg/syscall/syscall_plan...
src/pkg/syscall/syscall_plan9.go:336: func errstr() string {
I would:

func SyscallErrstrAsm(trap, a1, a2, a3 uintptr, ebuf *byte, ebuflen uint32) (r
uintptr)

func SyscallErrstr(trap, a1, a2, a3 uintptr) (r uintptr, err string) {
	b := make([]byte, ERRMAX)
	r = SyscallErrstrAsm(trap, a1, a2, a3, &b[0], uint32(len(b)))
	if b[0] == 0 {
		return r, ""
	}
	b[len(b)-1] = 0
	return r, string(b[:findnull(b)])
}

This way you don't need to call back from asm to go. Also you don't need to copy
errstr from buffer to buffer in asm.

[paulzhol, 2011-01-12 21:19:06]

synced with tip and added File.Sync.

[paulzhol, 2011-01-19 20:08:24]

Read #c/pid instead of accessing _tos.

[paulzhol, 2011-01-22 22:25:32]

Hi!
I finished ForkExec, please take another look.

Pavel

[paulzhol, 2011-02-02 16:52:27]

synced with tip

[rsc, 2011-02-09 05:27:37]

no hurry; just wanted to note that i'm
waiting for syscall to be split into a different
CL before reviewing this.

[paulzhol, 2011-02-09 09:07:35]

On 2011/02/09 05:27:37, rsc wrote:
> no hurry; just wanted to note that i'm
> waiting for syscall to be split into a different
> CL before reviewing this.

Done, os changes are in http://codereview.appspot.com/4149046/.

[ality, 2011-02-09 19:50:18]

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:99: r1, _, _ = RawSyscall(SYS_RFORK,
uintptr(RFPROC|RFFDG|RFREND|clearenv), 0, 0)
You should check if the rfork fails.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:119: r1, _, _ = RawSyscall(SYS_CREATE,
uintptr(unsafe.Pointer(envv[i].name)), uintptr(O_WRONLY), uintptr(0666))
I would split this chunk of code into
a setenv function so the error handling
is easier to understand.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:224: const (
Get rid of the parens in the const expression.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/mkall.sh
File src/pkg/syscall/mkall.sh (right):

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/mkall.sh#ne...
src/pkg/syscall/mkall.sh:160: mksysnum="./mksysnum_plan9.sh
/media/sys/src/libc/9syscall/sys.h"
Maybe change this to point to sources so others
know where to find it? /n/sources/plan9/...

Not terribly important, though.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:109: func atoi(buf []byte) int {
These two functions could be shorter if
you used named result parameters. E.g.,

func atoi(b []byte) (n int) {
    for i := 0; i < len(b); i++ {
        n = n*10 + int(b[i] - '0')
    }
    return n
}

func findnull(b []byte) (i int) {
    for ; i < len(b) && b[i] != '\0'; i++ {
    }
    return i
}

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:140: 
There are a few extraneous empty lines
in this and other files. They could go
away without impacting readability.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:233: return sleep(int32((nsec + 999) / 1e6))
Add the comment "round up to milliseconds"
to match the other syscall_$arch.go files.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:267: func Await(msg *Waitmsg) (err Error) {
You can avoid the append here by doing
something like the following:

func Await(w *Waitmsg) (err Error) {
    var buf [512]byte
    var f [5][]byte

    n, err := await(buf[:])
    if err != nil || w == nil {
        return
    }

    var nf, p int
    for i := 0; i < n && nf < len(f); i++ {
        if buf[i] == ' ' {
            f[nf] = buf[p:i]
            p = i+1
            nf++
        }
    }
    if nf != len(f) {
        return NewError("invalid wait message")
    }
    w.Pid = atoi(f[0])
    w.Time[0] = uint32(atoi(f[1]))
    w.Time[1] = uint32(atoi(f[2]))
    w.Time[2] = uint32(atoi(f[3]))
    w.Msg = string(f[4])
    return
}

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:340: fd, e := Open("#c/bintime", O_RDONLY)
This should be "/dev/bintime". Someone may have
bound in another file. Cinap has a pretty neat
utility called warpfs (on contrib) that does this.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:346: _, e = Pread(fd, b[:], 0)
Why not check that you've read at least 8 bytes?

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:367: err_buffer := make([]byte, ERRMAX)
Any reason this can't be "var buf [ERRMAX]byte"?

[paulzhol, 2011-02-10 08:41:39]

Thanks for the review! Please take another look.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:99: r1, _, _ = RawSyscall(SYS_RFORK,
uintptr(RFPROC|RFFDG|RFREND|clearenv), 0, 0)
On 2011/02/09 19:50:19, ality wrote:
> You should check if the rfork fails.

I've added an r1 == -1 test, but I'm not sure I can get the errstr here because
this is still running in the parent process with threads and locks. Can I safely
use SyscallErrstr here ?

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:119: r1, _, _ = RawSyscall(SYS_CREATE,
uintptr(unsafe.Pointer(envv[i].name)), uintptr(O_WRONLY), uintptr(0666))
On 2011/02/09 19:50:19, ality wrote:
> I would split this chunk of code into
> a setenv function so the error handling
> is easier to understand.

We are inside forkAndExecInChild after an rfork, therefore we cannot grow
stack/allocate memory. I would have to code setenv in C with #pragma textflag 7
set.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:224: const (
On 2011/02/09 19:50:19, ality wrote:
> Get rid of the parens in the const expression.

Done.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/mkall.sh
File src/pkg/syscall/mkall.sh (right):

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/mkall.sh#ne...
src/pkg/syscall/mkall.sh:160: mksysnum="./mksysnum_plan9.sh
/media/sys/src/libc/9syscall/sys.h"
On 2011/02/09 19:50:19, ality wrote:
> Maybe change this to point to sources so others
> know where to find it? /n/sources/plan9/...
> 
> Not terribly important, though.

Done.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:109: func atoi(buf []byte) int {
On 2011/02/09 19:50:19, ality wrote:
> These two functions could be shorter if
> you used named result parameters. E.g.,
> 
> func atoi(b []byte) (n int) {
>     for i := 0; i < len(b); i++ {
>         n = n*10 + int(b[i] - '0')
>     }
>     return n
> }
> 
> func findnull(b []byte) (i int) {
>     for ; i < len(b) && b[i] != '\0'; i++ {
>     }
>     return i
> }

Done.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:233: return sleep(int32((nsec + 999) / 1e6))
On 2011/02/09 19:50:19, ality wrote:
> Add the comment "round up to milliseconds"
> to match the other syscall_$arch.go files.

Done.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:267: func Await(msg *Waitmsg) (err Error) {
On 2011/02/09 19:50:19, ality wrote:
> You can avoid the append here by doing
> something like the following:
> 
> func Await(w *Waitmsg) (err Error) {
>     var buf [512]byte
>     var f [5][]byte
> 
>     n, err := await(buf[:])
>     if err != nil || w == nil {
>         return
>     }
> 
>     var nf, p int
>     for i := 0; i < n && nf < len(f); i++ {
>         if buf[i] == ' ' {
>             f[nf] = buf[p:i]
>             p = i+1
>             nf++
>         }
>     }
>     if nf != len(f) {
>         return NewError("invalid wait message")
>     }
>     w.Pid = atoi(f[0])
>     w.Time[0] = uint32(atoi(f[1]))
>     w.Time[1] = uint32(atoi(f[2]))
>     w.Time[2] = uint32(atoi(f[3]))
>     w.Msg = string(f[4])
>     return
> }

Done.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:340: fd, e := Open("#c/bintime", O_RDONLY)
On 2011/02/09 19:50:19, ality wrote:
> This should be "/dev/bintime". Someone may have
> bound in another file. Cinap has a pretty neat
> utility called warpfs (on contrib) that does this.

Done.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:346: _, e = Pread(fd, b[:], 0)
On 2011/02/09 19:50:19, ality wrote:
> Why not check that you've read at least 8 bytes?

Done.

http://codereview.appspot.com/3816043/diff/125001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:367: err_buffer := make([]byte, ERRMAX)
On 2011/02/09 19:50:19, ality wrote:
> Any reason this can't be "var buf [ERRMAX]byte"?

Done.

[rsc, 2011-02-11 19:31:35]

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/asm_plan9_3...
File src/pkg/syscall/asm_plan9_386.s (right):

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/asm_plan9_3...
src/pkg/syscall/asm_plan9_386.s:9: // func Syscall(trap int32, a1, a2, a3 int32)
(r1, r2, err uintptr);
I am not 100% sure but I think this is wrong.
It has the benefit of looking like the other systems, but it
implies that err uintptr is useful, when in fact it is not.

I'd rather not distinguish the two cases (syscall and syscallerrstr)
and instead just have

func Syscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err string)

Note that they should be uintptr, not int32.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/asm_plan9_3...
src/pkg/syscall/asm_plan9_386.s:162: TEXT ·seek(SB),7,$0
Why is this here?

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:1: // Copyright 2011 The Go Authors. All rights
reserved.
we don't update copyright dates
leave as 2009

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:84: // declarations require heap allocation (e.g.,
err1).
s/err1/errbuf/

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:103: return 0, NewError("rfork failed")
// Fork failed.  Allowed to call non-assembly functions.
return 0, NewError(errstr())

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:131: uintptr(offset), uintptr(offset>>32), 0)
^uintptr(0), ^uintptr(0)
then you can delete the unused offset var

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:204: errbuf[cap(errbuf)-1] = 0
s/cap/len/
there's no difference so it's confusing to see cap

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:205: for i = 0; i < len(errbuf) && errbuf[i] != 0;
i++ {
please make this

i = 0
for i < len(errbuf) {
    i++
}

just to emphasize that i++ is the point of the loop

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:225: func cexec_pipe(p []int) Error {
go names should be mixedCase not under_scores

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:226: const PIPEMNT = "/mnt/temp"
This has various races, especially because you're not using RawSyscall.
Easier way (maybe a bug in Plan 9, but it works):

p1, p2, err := Pipe()
fd, err := Open("#d/"+itoa(p2), O_CLOEXEC)
Close(fd)

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:372: func ForkExec(argv0 string, argv []string,
envv []string, dir string, fd []int, startup_fd_list, devdup Fdlist) (pid int,
err Error) {
I would rather not need to change ForkExec's signature.
Can we put the necessary functionality in syscall?
It should be easy to parse the directory read enough
to extract just the names.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:378: return 0, NewError("Ptrace not supported on
Plan 9")
s/Ptrace/&ForkExec/

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:383: _, e := SyscallErrstr(SYS_EXEC,
This ignores the setting of the environment.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/mksyscall_p...
File src/pkg/syscall/mksyscall_plan9.awk (right):

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/mksyscall_p...
src/pkg/syscall/mksyscall_plan9.awk:1: function parse_line(line) {
missing copyright header.
i would very much like this to be an hg cp of mksyscall.sh
and adapted as needed.  

the changes might be small enough to just have a -plan9 flag.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:291: func Unmount(name, old string) (err Error)
{
Why can't this one be auto-generated?

[paulzhol, 2011-02-12 15:59:38]

Thanks for the review!

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/asm_plan9_3...
File src/pkg/syscall/asm_plan9_386.s (right):

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/asm_plan9_3...
src/pkg/syscall/asm_plan9_386.s:162: TEXT ·seek(SB),7,$0
On 2011/02/11 19:31:35, rsc wrote:
> Why is this here?

I tried to avoid allocation of newoffset when taking it's address, with the
added benefit of not sliding the stack arguments.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/mksyscall_p...
File src/pkg/syscall/mksyscall_plan9.awk (right):

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/mksyscall_p...
src/pkg/syscall/mksyscall_plan9.awk:1: function parse_line(line) {
On 2011/02/11 19:31:35, rsc wrote:
> missing copyright header.
> i would very much like this to be an hg cp of mksyscall.sh
> and adapted as needed.  
> 
> the changes might be small enough to just have a -plan9 flag.

Originally I wrote the awk version, when I thought it would be trivial to update
the native Go port. I'll convert the relevant bits to perl.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:291: func Unmount(name, old string) (err Error)
{
On 2011/02/11 19:31:35, rsc wrote:
> Why can't this one be auto-generated?

If name is an empty string, namep will be 0, so unmount will unbind / unmount
everything mounted at old.

The auto-generated code for strings does not have this property as []byte does:
var _p0 unsafe.Pointer
if len(buf) > 0 {
  _p0 = unsafe.Pointer(&buf[0])
}

[paulzhol, 2011-02-13 20:42:58]

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/asm_plan9_3...
File src/pkg/syscall/asm_plan9_386.s (right):

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/asm_plan9_3...
src/pkg/syscall/asm_plan9_386.s:9: // func Syscall(trap int32, a1, a2, a3 int32)
(r1, r2, err uintptr);
On 2011/02/11 19:31:35, rsc wrote:
> I am not 100% sure but I think this is wrong.
> It has the benefit of looking like the other systems, but it
> implies that err uintptr is useful, when in fact it is not.
> 
> I'd rather not distinguish the two cases (syscall and syscallerrstr)
> and instead just have
> 
> func Syscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err string)
> 
> Note that they should be uintptr, not int32.
> 

Done.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:1: // Copyright 2011 The Go Authors. All rights
reserved.
On 2011/02/11 19:31:35, rsc wrote:
> we don't update copyright dates
> leave as 2009

Done.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:84: // declarations require heap allocation (e.g.,
err1).
On 2011/02/11 19:31:35, rsc wrote:
> s/err1/errbuf/

Done.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:103: return 0, NewError("rfork failed")
On 2011/02/11 19:31:35, rsc wrote:
> // Fork failed.  Allowed to call non-assembly functions.
> return 0, NewError(errstr())
> 

Done.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:131: uintptr(offset), uintptr(offset>>32), 0)
On 2011/02/11 19:31:35, rsc wrote:
> ^uintptr(0), ^uintptr(0)
> then you can delete the unused offset var

Done.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:204: errbuf[cap(errbuf)-1] = 0
On 2011/02/11 19:31:35, rsc wrote:
> s/cap/len/
> there's no difference so it's confusing to see cap

Done.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:205: for i = 0; i < len(errbuf) && errbuf[i] != 0;
i++ {
On 2011/02/11 19:31:35, rsc wrote:
> please make this
> 
> i = 0
> for i < len(errbuf) {
>     i++
> }
> 
> just to emphasize that i++ is the point of the loop

Done.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:225: func cexec_pipe(p []int) Error {
On 2011/02/11 19:31:35, rsc wrote:
> go names should be mixedCase not under_scores

Done.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:226: const PIPEMNT = "/mnt/temp"
On 2011/02/11 19:31:35, rsc wrote:
> This has various races, especially because you're not using RawSyscall.
> Easier way (maybe a bug in Plan 9, but it works):
> 
> p1, p2, err := Pipe()
> fd, err := Open("#d/"+itoa(p2), O_CLOEXEC)
> Close(fd)

Done.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:372: func ForkExec(argv0 string, argv []string,
envv []string, dir string, fd []int, startup_fd_list, devdup Fdlist) (pid int,
err Error) {
On 2011/02/11 19:31:35, rsc wrote:
> I would rather not need to change ForkExec's signature.
> Can we put the necessary functionality in syscall?
> It should be easy to parse the directory read enough
> to extract just the names.
> 

Done.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:378: return 0, NewError("Ptrace not supported on
Plan 9")
On 2011/02/11 19:31:35, rsc wrote:
> s/Ptrace/&ForkExec/

Done.

http://codereview.appspot.com/3816043/diff/133002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:383: _, e := SyscallErrstr(SYS_EXEC,
On 2011/02/11 19:31:35, rsc wrote:
> This ignores the setting of the environment.

Done.

[paulzhol, 2011-02-13 20:44:04]

Hi! Please take another look.

Pavel

[r2, 2011-02-18 00:35:36]

still absorbing it

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/asm_plan9_3...
File src/pkg/syscall/asm_plan9_386.s (right):

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/asm_plan9_3...
src/pkg/syscall/asm_plan9_386.s:125: CALL	runtime·entersyscall(SB)
rsc may disagree but since you're writing assembly and, i believe, this is a
syscall that cannot block (other than one very cheap mutex) in plan9, i don't
think you need this boilerplate.

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:83: func readdirnames(dirfd int) (dirnames
[]string, err Error) {
comments on all these functions would be nice

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:114: // ForkLock should be write locked.
this line should go outside into the (missing) func comment

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:46: // Plan 9 errors
do you need these?

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:116: func findnull(s []byte) (i int) {
for i = range s {
  if s[i] == '\0' {
    break
  }
}
return

moreover, since you always use this to make a string (i think), might as well do
that here and call it something like cstring(s)

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:128: return string(buf[:findnull(buf[:])])
see comments on fidnull

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:155: func readnum(path string) (int, Error) {
this doesn't read signed ints, so i'd say return uint instead, and convert in
the two places that care.

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:216: buf[len(buf)-1] = 0
not necessary. i wrote fd2path and i believe you always get a \0. the manual
agrees with me.

[paulzhol, 2011-02-21 20:18:06]

Thanks for the review!

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/asm_plan9_3...
File src/pkg/syscall/asm_plan9_386.s (right):

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/asm_plan9_3...
src/pkg/syscall/asm_plan9_386.s:125: CALL	runtime·entersyscall(SB)
On 2011/02/18 00:35:36, r2 wrote:
> rsc may disagree but since you're writing assembly and, i believe, this is a
> syscall that cannot block (other than one very cheap mutex) in plan9, i don't
> think you need this boilerplate.

Done.

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:83: func readdirnames(dirfd int) (dirnames
[]string, err Error) {
On 2011/02/18 00:35:36, r2 wrote:
> comments on all these functions would be nice

Done.

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:114: // ForkLock should be write locked.
On 2011/02/18 00:35:36, r2 wrote:
> this line should go outside into the (missing) func comment

Done.

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:46: // Plan 9 errors
On 2011/02/18 00:35:36, r2 wrote:
> do you need these?

I've used some of them (Ebadstat, Ebadarg, Enotdir, Enonexist, Eexist and
Ebadfd) in pkg os.
Should I keep only the one's I use? or drop all of them?

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:116: func findnull(s []byte) (i int) {
On 2011/02/18 00:35:36, r2 wrote:
> for i = range s {
>   if s[i] == '\0' {
>     break
>   }
> }
> return
> 
> moreover, since you always use this to make a string (i think), might as well
do
> that here and call it something like cstring(s)

Done.

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:128: return string(buf[:findnull(buf[:])])
On 2011/02/18 00:35:36, r2 wrote:
> see comments on fidnull

Done.

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:155: func readnum(path string) (int, Error) {
On 2011/02/18 00:35:36, r2 wrote:
> this doesn't read signed ints, so i'd say return uint instead, and convert in
> the two places that care.

Done.

http://codereview.appspot.com/3816043/diff/138016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:216: buf[len(buf)-1] = 0
On 2011/02/18 00:35:36, r2 wrote:
> not necessary. i wrote fd2path and i believe you always get a \0. the manual
> agrees with me.

Oops I missed that bit in the man, fixed.

[r, 2011-02-22 18:35:16]

coming along nicely

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:120: // Return a list of currently opened fd's
(excluding stdin, stdout, stderr)
s/'// (plural not possessive)

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:139: if (len(fdstr) > 2) &&
(string(fdstr[len(fdstr)-3:]) == "ctl") {
you're allocating in order to do a comparison.  write out the ctl check as
bytes.

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:177: var r1 uintptr
you have so many, might as well group the vars in ()

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:211: // Write new enviorment variables.
misspelled environment

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:352: var wmsg Waitmsg
var ()

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/mksysnum_pl...
File src/pkg/syscall/mksysnum_plan9.sh (right):

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/mksysnum_pl...
src/pkg/syscall/mksysnum_plan9.sh:19: cat $1 | sed -r 's/^#define[
\t]([A-Z0-9_]+)[ \t]+([0-9]+)/\tSYS_\1=\2/g' | grep -v SYS__
this could be
< $1  sed....
but you can leave it as is if you prefer

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:46: // Plan 9 errors
i still don't quite see why you need these. plan 9 errors are not literals like
this; they're packaged up by the kernel.  i don't have package os in front of me
to see how you use them, but go programmers won't use them as i understand it.

perhaps all that's needed is a comment explaining what they're for.

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:347: fd, e := Open("/dev/bintime", O_RDONLY)
this is fine if you want to leave it like this but it's punitively expensive for
such a common call.  still, programmers that care will open /dev/bintime and do
the work themselves.
so two suggestions:
1) put a TODO suggesting using lower-level calls to avoid the allocations. let's
not worry about doing it just yet
2) write a public helper to unpack a bintime record

[paulzhol, 2011-02-22 22:06:15]

Thanks!

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:120: // Return a list of currently opened fd's
(excluding stdin, stdout, stderr)
On 2011/02/22 18:35:16, r wrote:
> s/'// (plural not possessive)

Done.

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:139: if (len(fdstr) > 2) &&
(string(fdstr[len(fdstr)-3:]) == "ctl") {
On 2011/02/22 18:35:16, r wrote:
> you're allocating in order to do a comparison.  write out the ctl check as
> bytes.

Done, I also think e was allocated unnecessarily.

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:177: var r1 uintptr
On 2011/02/22 18:35:16, r wrote:
> you have so many, might as well group the vars in ()

Done.

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:211: // Write new enviorment variables.
On 2011/02/22 18:35:16, r wrote:
> misspelled environment

Done.

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:352: var wmsg Waitmsg
On 2011/02/22 18:35:16, r wrote:
> var ()

Done.

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/mksysnum_pl...
File src/pkg/syscall/mksysnum_plan9.sh (right):

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/mksysnum_pl...
src/pkg/syscall/mksysnum_plan9.sh:19: cat $1 | sed -r 's/^#define[
\t]([A-Z0-9_]+)[ \t]+([0-9]+)/\tSYS_\1=\2/g' | grep -v SYS__
On 2011/02/22 18:35:16, r wrote:
> this could be
> < $1  sed....
> but you can leave it as is if you prefer

Is it "better" because cat is not opening a new file descriptor, letting the
shell do that for it instead ?

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:46: // Plan 9 errors
On 2011/02/22 18:35:16, r wrote:
> i still don't quite see why you need these. plan 9 errors are not literals
like
> this; they're packaged up by the kernel.  i don't have package os in front of
me
> to see how you use them, but go programmers won't use them as i understand it.
> 
> perhaps all that's needed is a comment explaining what they're for.

Well, they aren't used in syscall (except for maybe Ebadstat in readdirnames) so
I'll remove them and make some changes to os.

http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:347: fd, e := Open("/dev/bintime", O_RDONLY)
On 2011/02/22 18:35:16, r wrote:
> this is fine if you want to leave it like this but it's punitively expensive
for
> such a common call.  still, programmers that care will open /dev/bintime and
do
> the work themselves.
> so two suggestions:
> 1) put a TODO suggesting using lower-level calls to avoid the allocations.
let's
> not worry about doing it just yet
> 2) write a public helper to unpack a bintime record

is var b [8]byte allocated because it is used in slice operations ? How can I
avoid this? rewrite with assembly/goc ?

[r2, 2011-02-23 00:42:33]

i think this is looking good. someone else should take a look.

http://codereview.appspot.com/3816043/diff/143016/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/143016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:283: func BintimeDecode(b []byte) (nsec int64,
err Error) {
i think DecodeBintime (or maybe DecodeBinTime) is better.
you could use a more generic name too, but this is fine.

http://codereview.appspot.com/3816043/diff/143016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:283: func BintimeDecode(b []byte) (nsec int64,
err Error) {
i think DecodeBintime (or maybe DecodeBinTime) is better.
you could use a more generic name too, but this is fine.

http://codereview.appspot.com/3816043/diff/143016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:313: if _, e = Pread(fd, b[:], 0); e != nil {
yes, this slicing operation allocates. it's hard to avoid that without a
permanent place to tuck the value away.  don't worry about it.

[r2, 2011-02-23 00:43:04]

On Feb 22, 2011, at 2:06 PM, paulzhol@gmail.com wrote:

>
http://codereview.appspot.com/3816043/diff/135042/src/pkg/syscall/mksysnum_pl...
> src/pkg/syscall/mksysnum_plan9.sh:19: cat $1 | sed -r 's/^#define[
> \t]([A-Z0-9_]+)[ \t]+([0-9]+)/\tSYS_\1=\2/g' | grep -v SYS__
> On 2011/02/22 18:35:16, r wrote:
>> this could be
>> < $1  sed....
>> but you can leave it as is if you prefer
> 
> Is it "better" because cat is not opening a new file descriptor, letting
> the shell do that for it instead ?

why start a process when you can just open a file?

-rob

[paulzhol, 2011-02-23 07:14:51]

> why start a process when you can just open a file?
> 
> -rob
> 

Right :)

[paulzhol, 2011-02-23 07:14:59]

http://codereview.appspot.com/3816043/diff/143016/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/143016/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:283: func BintimeDecode(b []byte) (nsec int64,
err Error) {
On 2011/02/23 00:42:33, r2 wrote:
> i think DecodeBintime (or maybe DecodeBinTime) is better.
> you could use a more generic name too, but this is fine.

Done.

[paulzhol, 2011-03-26 15:04:41]

synced ProcAttr changes, PTAL.

[r, 2011-03-31 22:51:24]

sorry for the delay.
these are mostly cosmetic. it's close to ready.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:74: // Read a 16-bit numeric value from a 9P
use godoc commenting convention. see eample for readdirname

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:81: // Read a string from a 9P protocol message b,
ditto.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:89: // already opened for reading dirfd.
// readdirnames returns the names of files inside the directory represented by
dirfd.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:120: // Return a list of currently opened fds
(excluding stdin, stdout, stderr)
ditto

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:160: // inside startup_fds, and skip them when
manually closing descriptors
s/,//

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:161: // not ment to be passed to a child exec.
s/ment/meant/

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:166: // Fork, dup fd onto 0..len(fd), and
exec(argv0, argvv, envv) in child.
comment convention again.  (see recent golang.org blog post for more info)

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:388: envv_parsed = make([]envItem, 0,
len(attr.Env))
here and lots of other places: use camelCase not under_score.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:417: fds_to_close = append(fds_to_close[:i],
fds_to_close[i+1:]...)
this could be n^2 and very expensive. better to build a second slice. start with
fds := make([]int, 0, len(fds_to_close))

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:487: // Ordinary exec.
Exec is the regular exec function.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/mksysnum_pl...
File src/pkg/syscall/mksysnum_plan9.sh (right):

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/mksysnum_pl...
src/pkg/syscall/mksysnum_plan9.sh:19: sed -r 's/^#define[ \t]([A-Z0-9_]+)[
\t]+([0-9]+)/\tSYS_\1=\2/g' < $1 | grep -v SYS__
sed doesn't have + outside of plan 9 (stupid, i know).
to make it easy to cross-build if necessary, please use only "standard" sed
patterns

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:62: return ""
is that right?  shouldn't it be string(s) + "\000"?

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/types_plan9.c
File src/pkg/syscall/types_plan9.c (right):

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/types_plan9...
src/pkg/syscall/types_plan9.c:19: OREAD   = 0,       /* open for read */
//-style comments are more idiomatic

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/types_plan9...
src/pkg/syscall/types_plan9.c:61: /* bits in Qid.type */
indent

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/types_plan9...
src/pkg/syscall/types_plan9.c:86: QIDSZ = (BIT8SZ+BIT32SZ+BIT64SZ),
no need for parens.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/types_plan9...
src/pkg/syscall/types_plan9.c:90: $STATFIXLEN =
(BIT16SZ+QIDSZ+5*BIT16SZ+4*BIT32SZ+1*BIT64SZ),	/* amount of fixed length data in
a stat buffer */
ditto

[paulzhol, 2011-04-01 19:07:59]

Thanks! PTAL

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9.go
File src/pkg/syscall/exec_plan9.go (right):

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:74: // Read a 16-bit numeric value from a 9P
On 2011/03/31 22:51:24, r wrote:
> use godoc commenting convention. see eample for readdirname

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:81: // Read a string from a 9P protocol message b,
On 2011/03/31 22:51:24, r wrote:
> ditto.

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:89: // already opened for reading dirfd.
On 2011/03/31 22:51:24, r wrote:
> // readdirnames returns the names of files inside the directory represented by
> dirfd.

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:120: // Return a list of currently opened fds
(excluding stdin, stdout, stderr)
On 2011/03/31 22:51:24, r wrote:
> ditto

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:160: // inside startup_fds, and skip them when
manually closing descriptors
On 2011/03/31 22:51:24, r wrote:
> s/,//

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:161: // not ment to be passed to a child exec.
On 2011/03/31 22:51:24, r wrote:
> s/ment/meant/

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:166: // Fork, dup fd onto 0..len(fd), and
exec(argv0, argvv, envv) in child.
On 2011/03/31 22:51:24, r wrote:
> comment convention again.  (see recent http://golang.org blog post for more
info)

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:388: envv_parsed = make([]envItem, 0,
len(attr.Env))
On 2011/03/31 22:51:24, r wrote:
> here and lots of other places: use camelCase not under_score.

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:417: fds_to_close = append(fds_to_close[:i],
fds_to_close[i+1:]...)
On 2011/03/31 22:51:24, r wrote:
> this could be n^2 and very expensive. better to build a second slice. start
with
> fds := make([]int, 0, len(fds_to_close))

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/exec_plan9....
src/pkg/syscall/exec_plan9.go:487: // Ordinary exec.
On 2011/03/31 22:51:24, r wrote:
> Exec is the regular exec function.

I've replaced the RawSyscall calls with their wrapped counterparts when writing
the envv. 
Did I understand correctly or should I drop the envv handling entirely and just
call the SYS_EXEC syscall ?

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/mksysnum_pl...
File src/pkg/syscall/mksysnum_plan9.sh (right):

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/mksysnum_pl...
src/pkg/syscall/mksysnum_plan9.sh:19: sed -r 's/^#define[ \t]([A-Z0-9_]+)[
\t]+([0-9]+)/\tSYS_\1=\2/g' < $1 | grep -v SYS__
On 2011/03/31 22:51:24, r wrote:
> sed doesn't have + outside of plan 9 (stupid, i know).
> to make it easy to cross-build if necessary, please use only "standard" sed
> patterns

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:62: return ""
On 2011/03/31 22:51:24, r wrote:
> is that right?  shouldn't it be string(s) + "\000"?

Thanks, it should be just string(s)

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/types_plan9.c
File src/pkg/syscall/types_plan9.c (right):

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/types_plan9...
src/pkg/syscall/types_plan9.c:19: OREAD   = 0,       /* open for read */
On 2011/03/31 22:51:24, r wrote:
> //-style comments are more idiomatic

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/types_plan9...
src/pkg/syscall/types_plan9.c:61: /* bits in Qid.type */
On 2011/03/31 22:51:24, r wrote:
> indent

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/types_plan9...
src/pkg/syscall/types_plan9.c:86: QIDSZ = (BIT8SZ+BIT32SZ+BIT64SZ),
On 2011/03/31 22:51:24, r wrote:
> no need for parens.

Done.

http://codereview.appspot.com/3816043/diff/176002/src/pkg/syscall/types_plan9...
src/pkg/syscall/types_plan9.c:90: $STATFIXLEN =
(BIT16SZ+QIDSZ+5*BIT16SZ+4*BIT32SZ+1*BIT64SZ),	/* amount of fixed length data in
a stat buffer */
On 2011/03/31 22:51:24, r wrote:
> ditto

Done.

[rsc, 2011-04-01 21:33:33]

LGTM

Leaving for Rob.

http://codereview.appspot.com/3816043/diff/192001/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/192001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:20: // shamelessly copied from os.error.
d

http://codereview.appspot.com/3816043/diff/192001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:26: // A helper type that can be embedded or
wrapped to simplify satisfying
// ErrorString implements Error's String method by returning itself.

http://codereview.appspot.com/3816043/diff/192001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:75: * Wrapped
delete this comment

http://codereview.appspot.com/3816043/diff/192001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:94: msg := str(code)
not sure where str is defined but please rename it to itoa.

[paulzhol, 2011-04-01 22:00:10]

http://codereview.appspot.com/3816043/diff/192001/src/pkg/syscall/syscall_pla...
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/3816043/diff/192001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:20: // shamelessly copied from os.error.
On 2011/04/01 21:33:33, rsc wrote:
> d

Done.

http://codereview.appspot.com/3816043/diff/192001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:26: // A helper type that can be embedded or
wrapped to simplify satisfying
On 2011/04/01 21:33:33, rsc wrote:
> // ErrorString implements Error's String method by returning itself.

Done.

http://codereview.appspot.com/3816043/diff/192001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:75: * Wrapped
On 2011/04/01 21:33:33, rsc wrote:
> delete this comment

Done.

http://codereview.appspot.com/3816043/diff/192001/src/pkg/syscall/syscall_pla...
src/pkg/syscall/syscall_plan9.go:94: msg := str(code)
On 2011/04/01 21:33:33, rsc wrote:
> not sure where str is defined but please rename it to itoa.

str() is in str.go and also used by:

syscall_linux.go:63: return Utimes("/proc/self/fd/"+str(fd), tv)
syscall_unix.go:20: return "error " + str(errno)
syscall_windows.go:178: return "error " + str(errno) + " (FormatMessage failed
with err=" + str(err) + ")"

I'll rename to itoa and drop mine from exec_plan9.go

[r2, 2011-04-02 19:39:37]

looks like you've updated, but i didn't get a PTAL mail. is it ready for review?

-rob

[paulzhol, 2011-04-02 19:44:08]

On 2011/04/02 19:39:37, r2 wrote:
> looks like you've updated, but i didn't get a PTAL mail. is it ready for
review?
> 
> -rob
> 

Yes it is ready, sorry. I thought Russ needs to OK it first.

[r, 2011-04-02 19:54:53]

one small portability issue

http://codereview.appspot.com/3816043/diff/199001/src/pkg/syscall/mksysnum_pl...
File src/pkg/syscall/mksysnum_plan9.sh (right):

http://codereview.appspot.com/3816043/diff/199001/src/pkg/syscall/mksysnum_pl...
src/pkg/syscall/mksysnum_plan9.sh:19: sed 's/^#define[
\t]\([A-Z0-9_][A-Z0-9_]*\)[ \t][ \t]*\([0-9][0-9]*\)/\tSYS_\1=\2/g' < $1 | grep
-v SYS__
\t is not a general sed feature.
i suggest doing
SP='[   ]' #  space or tab
(use a tab character explicitly, of course)
and using the macro in there, for clarity.

[paulzhol, 2011-04-02 20:42:05]

On 2011/04/02 19:54:53, r wrote:
> one small portability issue
> 
>
http://codereview.appspot.com/3816043/diff/199001/src/pkg/syscall/mksysnum_pl...
> File src/pkg/syscall/mksysnum_plan9.sh (right):
> 
>
http://codereview.appspot.com/3816043/diff/199001/src/pkg/syscall/mksysnum_pl...
> src/pkg/syscall/mksysnum_plan9.sh:19: sed 's/^#define[
> \t]\([A-Z0-9_][A-Z0-9_]*\)[ \t][ \t]*\([0-9][0-9]*\)/\tSYS_\1=\2/g' < $1 |
grep
> -v SYS__
> \t is not a general sed feature.
> i suggest doing
> SP='[   ]' #  space or tab
> (use a tab character explicitly, of course)
> and using the macro in there, for clarity.

Done, PTAL

[r, 2011-04-02 20:49:25]

http://codereview.appspot.com/3816043/diff/198004/src/pkg/syscall/mksysnum_pl...
File src/pkg/syscall/mksysnum_plan9.sh (right):

http://codereview.appspot.com/3816043/diff/198004/src/pkg/syscall/mksysnum_pl...
src/pkg/syscall/mksysnum_plan9.sh:20: sed
"s/^#define${SP}\([A-Z0-9_][A-Z0-9_]*\)${SP}${SP}*\([0-9][0-9]*\)/SYS_\1=\2/g" \
sorry but not yet. inside "" the backslashes will be interpreted. you need to
double them.

[paulzhol, 2011-04-02 20:53:46]

On 2011/04/02 20:49:25, r wrote:
>
http://codereview.appspot.com/3816043/diff/198004/src/pkg/syscall/mksysnum_pl...
> File src/pkg/syscall/mksysnum_plan9.sh (right):
> 
>
http://codereview.appspot.com/3816043/diff/198004/src/pkg/syscall/mksysnum_pl...
> src/pkg/syscall/mksysnum_plan9.sh:20: sed
> "s/^#define${SP}\([A-Z0-9_][A-Z0-9_]*\)${SP}${SP}*\([0-9][0-9]*\)/SYS_\1=\2/g"
\
> sorry but not yet. inside "" the backslashes will be interpreted. you need to
> double them.

Done, PTAL

[r, 2011-04-02 21:20:33]

LGTM

stuff is sure to break but let's get this in

[paulzhol, 2011-04-02 21:23:33]

On Sun, Apr 3, 2011 at 12:20 AM,  <r@golang.org> wrote:
> LGTM
>
> stuff is sure to break but let's get this in

:) fingers crossed

[r, 2011-04-02 21:24:10]

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



R=rsc, brainman, ality, r2, r
CC=golang-dev
http://codereview.appspot.com/3816043

Committer: Rob Pike <r@golang.org>