code review 4149046: os: Plan 9 support.

os: Plan 9 support.

[paulzhol, 2011-02-09 09:06:38]

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

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

[ality, 2011-02-09 21:09:45]

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/dir_plan9.go
File src/pkg/os/dir_plan9.go (right):

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/dir_plan9.go#newco...
src/pkg/os/dir_plan9.go:11: const (
This is not needed.

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/dir_plan9.go#newco...
src/pkg/os/dir_plan9.go:24: func (file *File) Readdir(count int) (fi []FileInfo,
err Error) {
I think it'd be better to copy the unix Readdir
into file_plan9.go and do the real work here in
Readdirnames.

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/dir_plan9.go#newco...
src/pkg/os/dir_plan9.go:122: var nullDir = Dir{
All the functions in file_plan9.go refer to
this specific nullDir, which isn't correct.
Nulldir should be a function that either
returns a newly allocated Dir or sets up the
fields on a Dir pointer. I.e.,

    func nulldir() *Dir
        -- or --
    func nulldir(d *Dir)

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

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/env_plan9.go#newcode5
src/pkg/os/env_plan9.go:5: // Plan 9 environment variables.
A few nits:
  - All references to the #e device should
    be replaced with /env.

  - Is it even worth it to keep a cache of
    the environment variables on Plan 9?

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/file_plan9.go
File src/pkg/os/file_plan9.go (right):

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/file_plan9.go#newc...
src/pkg/os/file_plan9.go:18: const DevNull = "#c/null"
This should be "/dev/null".

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/sys_plan9.go
File src/pkg/os/sys_plan9.go (right):

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/sys_plan9.go#newco...
src/pkg/os/sys_plan9.go:11: f, err := Open("#e/sysname", O_RDONLY, 0)
This should be "#c/sysname".

[r, 2011-02-09 23:46:02]

a few comments

i missed the definition of iserror.  if we stay with that approach, which i need
to think about, it should be called isError

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

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/env_plan9.go#newcode1
src/pkg/os/env_plan9.go:1: // Copyright 2010 The Go Authors.  All rights
reserved.
2011

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/error_plan9.go
File src/pkg/os/error_plan9.go (right):

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/error_plan9.go#new...
src/pkg/os/error_plan9.go:22: // with the given system call name and error
number.
maybe s/number/details/ in all the error_*.go files

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/error_plan9.go#new...
src/pkg/os/error_plan9.go:23: // As a convenience, if errno is 0,
NewSyscallError returns nil.
what's errno?

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/file_plan9.go
File src/pkg/os/file_plan9.go (right):

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/file_plan9.go#newc...
src/pkg/os/file_plan9.go:119: func Rename(oldname, newname string) Error {
these functions should be commented

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/file_plan9.go#newc...
src/pkg/os/file_plan9.go:176: return nil, nil, NewSyscallError("pipe", e)
need to unlock

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/stat_plan9.go
File src/pkg/os/stat_plan9.go (right):

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/stat_plan9.go#newc...
src/pkg/os/stat_plan9.go:1: // Copyright 2010 The Go Authors. All rights
reserved.
2011

[paulzhol, 2011-02-10 22:59:38]

Thanks for the reviews!

iserror is defined in error_posix.go as:
func iserror(errno int) bool {
  return errno != 0	
}

it should be linked in by every os implementation which gets errno's from its
syscall package.

for Plan 9, iserror is defined in error_plan9.go:
func iserror(err syscall.Error) bool {
  return err != nil
}

it is expecting its syscall to return an Error interface with a String() method
when an error occurs or nil. The String() is usually a stored value of errstr
which is saved at the point of failure (syscall/asm_plan9_386.s SyscallErrstr
calls syscall/syscall_plan9.go errstr() if AX == -1).

Pavel

[paulzhol, 2011-02-12 14:49:02]

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/dir_plan9.go
File src/pkg/os/dir_plan9.go (right):

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/dir_plan9.go#newco...
src/pkg/os/dir_plan9.go:11: const (
On 2011/02/09 21:09:45, ality wrote:
> This is not needed.

Done.

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/dir_plan9.go#newco...
src/pkg/os/dir_plan9.go:24: func (file *File) Readdir(count int) (fi []FileInfo,
err Error) {
On 2011/02/09 21:09:45, ality wrote:
> I think it'd be better to copy the unix Readdir
> into file_plan9.go and do the real work here in
> Readdirnames.

I'm not sure I understand what you mean - the unix Readdir relies on
Readdirnames to get a list of all the filenames, and then calls stat on each. We
can get that data directly.

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/dir_plan9.go#newco...
src/pkg/os/dir_plan9.go:122: var nullDir = Dir{
On 2011/02/09 21:09:45, ality wrote:
> All the functions in file_plan9.go refer to
> this specific nullDir, which isn't correct.
> Nulldir should be a function that either
> returns a newly allocated Dir or sets up the
> fields on a Dir pointer. I.e.,
> 
>     func nulldir() *Dir
>         -- or --
>     func nulldir(d *Dir)

Done.

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

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/env_plan9.go#newcode1
src/pkg/os/env_plan9.go:1: // Copyright 2010 The Go Authors.  All rights
reserved.
On 2011/02/09 23:46:02, r wrote:
> 2011

Done.

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/env_plan9.go#newcode5
src/pkg/os/env_plan9.go:5: // Plan 9 environment variables.
On 2011/02/09 21:09:45, ality wrote:
> A few nits:
>   - All references to the #e device should
>     be replaced with /env.
> 
>   - Is it even worth it to keep a cache of
>     the environment variables on Plan 9?

Done.

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/error_plan9.go
File src/pkg/os/error_plan9.go (right):

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/error_plan9.go#new...
src/pkg/os/error_plan9.go:22: // with the given system call name and error
number.
On 2011/02/09 23:46:02, r wrote:
> maybe s/number/details/ in all the error_*.go files

Done.

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/error_plan9.go#new...
src/pkg/os/error_plan9.go:23: // As a convenience, if errno is 0,
NewSyscallError returns nil.
On 2011/02/09 23:46:02, r wrote:
> what's errno?

copy-paste, changed to err and nil.

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/file_plan9.go
File src/pkg/os/file_plan9.go (right):

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/file_plan9.go#newc...
src/pkg/os/file_plan9.go:18: const DevNull = "#c/null"
On 2011/02/09 21:09:45, ality wrote:
> This should be "/dev/null".

Done.

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/file_plan9.go#newc...
src/pkg/os/file_plan9.go:119: func Rename(oldname, newname string) Error {
On 2011/02/09 23:46:02, r wrote:
> these functions should be commented

Done.

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/file_plan9.go#newc...
src/pkg/os/file_plan9.go:176: return nil, nil, NewSyscallError("pipe", e)
On 2011/02/09 23:46:02, r wrote:
> need to unlock

Done.

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/stat_plan9.go
File src/pkg/os/stat_plan9.go (right):

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/stat_plan9.go#newc...
src/pkg/os/stat_plan9.go:1: // Copyright 2010 The Go Authors. All rights
reserved.
On 2011/02/09 23:46:02, r wrote:
> 2011

Done.

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/sys_plan9.go
File src/pkg/os/sys_plan9.go (right):

http://codereview.appspot.com/4149046/diff/4001/src/pkg/os/sys_plan9.go#newco...
src/pkg/os/sys_plan9.go:11: f, err := Open("#e/sysname", O_RDONLY, 0)
On 2011/02/09 21:09:45, ality wrote:
> This should be "#c/sysname".

Done.

[paulzhol, 2011-02-26 15:47:28]

sync with tip

[paulzhol, 2011-03-26 15:05:15]

synced ProcAttr changes, PTAL.

[r, 2011-03-31 22:57:41]

http://codereview.appspot.com/4149046/diff/41001/src/pkg/os/dir_plan9.go
File src/pkg/os/dir_plan9.go (right):

http://codereview.appspot.com/4149046/diff/41001/src/pkg/os/dir_plan9.go#newc...
src/pkg/os/dir_plan9.go:69: if count > 0 && len(result) >= count {
comment says negative, this says something else

http://codereview.appspot.com/4149046/diff/41001/src/pkg/os/dir_plan9.go#newc...
src/pkg/os/dir_plan9.go:80: func (file *File) Readdirnames(count int) (names
[]string, err Error) {
add doc comments throughout, and field comments on struct fields

http://codereview.appspot.com/4149046/diff/41001/src/pkg/os/stat_plan9.go
File src/pkg/os/stat_plan9.go (right):

http://codereview.appspot.com/4149046/diff/41001/src/pkg/os/stat_plan9.go#new...
src/pkg/os/stat_plan9.go:79: func Lstat(name string) (fi *FileInfo, err Error) {
copy the comment for Lstat from file.go

[paulzhol, 2011-04-01 20:16:44]

Thanks!

http://codereview.appspot.com/4149046/diff/41001/src/pkg/os/dir_plan9.go
File src/pkg/os/dir_plan9.go (right):

http://codereview.appspot.com/4149046/diff/41001/src/pkg/os/dir_plan9.go#newc...
src/pkg/os/dir_plan9.go:69: if count > 0 && len(result) >= count {
On 2011/03/31 22:57:46, r wrote:
> comment says negative, this says something else

if count is negative && will short-circuit the len(result) test and the loop
will continue until EOF, should I add a comment ?

http://codereview.appspot.com/4149046/diff/41001/src/pkg/os/dir_plan9.go#newc...
src/pkg/os/dir_plan9.go:80: func (file *File) Readdirnames(count int) (names
[]string, err Error) {
On 2011/03/31 22:57:46, r wrote:
> add doc comments throughout, and field comments on struct fields

Done.

Should Dir, Null() and UnmarshallDir() be exporte by package os ? or should they
be made private so goplan9 will be the reference implantation ?

http://codereview.appspot.com/4149046/diff/41001/src/pkg/os/stat_plan9.go
File src/pkg/os/stat_plan9.go (right):

http://codereview.appspot.com/4149046/diff/41001/src/pkg/os/stat_plan9.go#new...
src/pkg/os/stat_plan9.go:79: func Lstat(name string) (fi *FileInfo, err Error) {
On 2011/03/31 22:57:46, r wrote:
> copy the comment for Lstat from file.go

Done.

[rsc, 2011-04-01 21:37:28]

LGTM

Leaving for Rob.

http://codereview.appspot.com/4149046/diff/49001/src/pkg/os/error_plan9.go
File src/pkg/os/error_plan9.go (right):

http://codereview.appspot.com/4149046/diff/49001/src/pkg/os/error_plan9.go#ne...
src/pkg/os/error_plan9.go:32: Eshortstat Error = NewError("stat buffer too
small")
s/Error =/=/ throughout

[paulzhol, 2011-04-01 22:05:43]

http://codereview.appspot.com/4149046/diff/49001/src/pkg/os/error_plan9.go
File src/pkg/os/error_plan9.go (right):

http://codereview.appspot.com/4149046/diff/49001/src/pkg/os/error_plan9.go#ne...
src/pkg/os/error_plan9.go:32: Eshortstat Error = NewError("stat buffer too
small")
On 2011/04/01 21:37:28, rsc wrote:
> s/Error =/=/ throughout

Done.

[r, 2011-04-01 22:26:42]

LGTM
woo hoo

[r2, 2011-04-01 22:29:39]

waiting for the syscall changes before committing
(3816043)

-rob

[r, 2011-04-02 21:29:02]

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

os: Plan 9 support.

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

Committer: Rob Pike <r@golang.org>