code review 5411046: pkg/syscall: Adjustments for "errors" package, focussed...

pkg/syscall: Adjustments for "errors" package, focussed on Plan 9

exec_plan9.go:
. Adjusted return argument to match other changes.
#mksyscall.pl:
. Replaced "err = e1" with "err = NewError(e1)".
* Change abandoned, Russ made a better suggestion involving
  syscall_plan9.go.
syscall_plan9.go:
. Removed redundant "err = nil" lines.
. Adjusted //sys lines for mksyscall.pl.
* Replaced "err string" with "err ErrorString" in return arguments.
zsyscall_plan9_386.go:
. This module ought to be generated, but as it exists in the
  repository, I rebuilt it and checked that it matched expectations.
  Anybody is welcome to remove this from the repository if
  they feel it should go, but remember that not all Plan 9
  installations have a working Perl.

[lucio, 2011-11-19 04:19:27]

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

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

[lucio, 2011-11-19 04:20:29]

The CL comment was confusing.

Lucio.

On 11/19/11, lucio.dere@gmail.com <lucio.dere@gmail.com> wrote:
> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: ality@pbrane.org,
> golang-dev@googlegroups.com, rsc@golang.org),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> pkg/syscall: Adjustments for "errors" package, focussed on Plan 9
>
> mksyscall.pl:
> . Replaced "err = e1" with "err = NewError(e1)".
> syscall_plan9.go:
> . Removed redundant "err = nil" lines.
> . Adjusted //sys lines for mksyscall.pl.
> zsyscall_plan9_386.go:
> . This module ought to be generated, but as it exists in the
>    repository, I rebuilt it and checked that it matched expectations.
>    Anybody is welcome to remove this from the repository if
>    they feel it should go, but remember that not all Plan 9
>    installations have a working Perl.
>
> Please review this at http://codereview.appspot.com/5411046/
>
> Affected files:
>    M src/pkg/syscall/mksyscall.pl
>    M src/pkg/syscall/syscall_plan9.go
>    M src/pkg/syscall/zsyscall_plan9_386.go
>
>
> Index: src/pkg/syscall/mksyscall.pl
> ===================================================================
> --- a/src/pkg/syscall/mksyscall.pl
> +++ b/src/pkg/syscall/mksyscall.pl
> @@ -223,11 +223,11 @@
>   	
>   	if ($plan9 && $ret[2] eq "e1") {
>   		$text .= "\tif int(r0) == -1 {\n";
> -		$text .= "\t\terr = e1\n";
> +		$text .= "\t\terr = NewError(e1)\n";
>   		$text .= "\t}\n";
>   	} elsif ($do_errno) {
>   		$text .= "\tif e1 != 0 {\n";
> -		$text .= "\t\terr = e1;\n";
> +		$text .= "\t\terr = NewError(e1);\n";
>   		$text .= "\t}\n";
>   	}
>   	$text .= "\treturn\n";
> Index: src/pkg/syscall/syscall_plan9.go
> ===================================================================
> --- a/src/pkg/syscall/syscall_plan9.go
> +++ b/src/pkg/syscall/syscall_plan9.go
> @@ -170,7 +170,6 @@
>   func Seek(fd int, offset int64, whence int) (newoffset int64, err error) {
>   	newoffset, e := seek(0, fd, offset, whence)
>
> -	err = nil
>   	if newoffset == -1 {
>   		err = NewError(e)
>   	}
> @@ -255,7 +254,6 @@
>   		r0, _, e = Syscall(SYS_UNMOUNT,
> uintptr(unsafe.Pointer(StringBytePtr(name))), oldp, 0)
>   	}
>
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e)
>   	}
> @@ -288,7 +286,6 @@
>   	if len(b) != 8 {
>   		return -1, NewError("bad /dev/bintime format")
>   	}
> -	err = nil
>   	nsec = int64(b[0])<<56 |
>   		int64(b[1])<<48 |
>   		int64(b[2])<<40 |
> @@ -335,17 +332,17 @@
>   	return make([]int, 0), nil
>   }
>
> -//sys	Dup(oldfd int, newfd int) (fd int, err Error)
> -//sys	Open(path string, mode int) (fd int, err Error)
> -//sys	Create(path string, mode int, perm uint32) (fd int, err Error)
> -//sys	Remove(path string) (err Error)
> -//sys	Pread(fd int, p []byte, offset int64) (n int, err Error)
> -//sys	Pwrite(fd int, p []byte, offset int64) (n int, err Error)
> -//sys	Close(fd int) (err Error)
> -//sys	Chdir(path string) (err Error)
> -//sys	Bind(name string, old string, flag int) (err Error)
> -//sys	Mount(fd int, afd int, old string, flag int, aname string) (err
> Error)
> -//sys	Stat(path string, edir []byte) (n int, err Error)
> -//sys	Fstat(fd int, edir []byte) (n int, err Error)
> -//sys	Wstat(path string, edir []byte) (err Error)
> -//sys	Fwstat(fd int, edir []byte) (err Error)
> +//sys	Dup(oldfd int, newfd int) (fd int, err error)
> +//sys	Open(path string, mode int) (fd int, err error)
> +//sys	Create(path string, mode int, perm uint32) (fd int, err error)
> +//sys	Remove(path string) (err error)
> +//sys	Pread(fd int, p []byte, offset int64) (n int, err error)
> +//sys	Pwrite(fd int, p []byte, offset int64) (n int, err error)
> +//sys	Close(fd int) (err error)
> +//sys	Chdir(path string) (err error)
> +//sys	Bind(name string, old string, flag int) (err error)
> +//sys	Mount(fd int, afd int, old string, flag int, aname string) (err
> error)
> +//sys	Stat(path string, edir []byte) (n int, err error)
> +//sys	Fstat(fd int, edir []byte) (n int, err error)
> +//sys	Wstat(path string, edir []byte) (err error)
> +//sys	Fwstat(fd int, edir []byte) (err error)
> Index: src/pkg/syscall/zsyscall_plan9_386.go
> ===================================================================
> --- a/src/pkg/syscall/zsyscall_plan9_386.go
> +++ b/src/pkg/syscall/zsyscall_plan9_386.go
> @@ -22,7 +22,6 @@
>   		_p0 = unsafe.Pointer(&_zero)
>   	}
>   	r0, _, e1 := Syscall(SYS_FD2PATH, uintptr(fd), uintptr(_p0),
> uintptr(len(buf)))
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -33,18 +32,6 @@
>
>   func pipe(p *[2]_C_int) (err error) {
>   	r0, _, e1 := Syscall(SYS_PIPE, uintptr(unsafe.Pointer(p)), 0, 0)
> -	err = nil
> -	if int(r0) == -1 {
> -		err = NewError(e1)
> -	}
> -	return
> -}
> -
> -// THIS FILE IS GENERATED BY THE COMMAND AT THE TOP; DO NOT EDIT
> -
> -func sleep(millisecs int32) (err error) {
> -	r0, _, e1 := Syscall(SYS_SLEEP, uintptr(millisecs), 0, 0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -62,7 +49,6 @@
>   	}
>   	r0, _, e1 := Syscall(SYS_AWAIT, uintptr(_p0), uintptr(len(s)), 0)
>   	n = int(r0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -74,7 +60,6 @@
>   func Dup(oldfd int, newfd int) (fd int, err error) {
>   	r0, _, e1 := Syscall(SYS_DUP, uintptr(oldfd), uintptr(newfd), 0)
>   	fd = int(r0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -86,7 +71,6 @@
>   func Open(path string, mode int) (fd int, err error) {
>   	r0, _, e1 := Syscall(SYS_OPEN,
> uintptr(unsafe.Pointer(StringBytePtr(path))), uintptr(mode), 0)
>   	fd = int(r0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -98,7 +82,6 @@
>   func Create(path string, mode int, perm uint32) (fd int, err error) {
>   	r0, _, e1 := Syscall(SYS_CREATE,
> uintptr(unsafe.Pointer(StringBytePtr(path))), uintptr(mode), uintptr(perm))
>   	fd = int(r0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -109,7 +92,6 @@
>
>   func Remove(path string) (err error) {
>   	r0, _, e1 := Syscall(SYS_REMOVE,
> uintptr(unsafe.Pointer(StringBytePtr(path))), 0, 0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -127,7 +109,6 @@
>   	}
>   	r0, _, e1 := Syscall6(SYS_PREAD, uintptr(fd), uintptr(_p0),
> uintptr(len(p)), uintptr(offset), uintptr(offset>>32), 0)
>   	n = int(r0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -145,7 +126,6 @@
>   	}
>   	r0, _, e1 := Syscall6(SYS_PWRITE, uintptr(fd), uintptr(_p0),
> uintptr(len(p)), uintptr(offset), uintptr(offset>>32), 0)
>   	n = int(r0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -156,7 +136,6 @@
>
>   func Close(fd int) (err error) {
>   	r0, _, e1 := Syscall(SYS_CLOSE, uintptr(fd), 0, 0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -167,7 +146,6 @@
>
>   func Chdir(path string) (err error) {
>   	r0, _, e1 := Syscall(SYS_CHDIR,
> uintptr(unsafe.Pointer(StringBytePtr(path))), 0, 0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -178,7 +156,6 @@
>
>   func Bind(name string, old string, flag int) (err error) {
>   	r0, _, e1 := Syscall(SYS_BIND,
> uintptr(unsafe.Pointer(StringBytePtr(name))),
> uintptr(unsafe.Pointer(StringBytePtr(old))), uintptr(flag))
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -189,7 +166,6 @@
>
>   func Mount(fd int, afd int, old string, flag int, aname string) (err
> error) {
>   	r0, _, e1 := Syscall6(SYS_MOUNT, uintptr(fd), uintptr(afd),
> uintptr(unsafe.Pointer(StringBytePtr(old))), uintptr(flag),
> uintptr(unsafe.Pointer(StringBytePtr(aname))), 0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -207,7 +183,6 @@
>   	}
>   	r0, _, e1 := Syscall(SYS_STAT,
> uintptr(unsafe.Pointer(StringBytePtr(path))), uintptr(_p0),
> uintptr(len(edir)))
>   	n = int(r0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -225,7 +200,6 @@
>   	}
>   	r0, _, e1 := Syscall(SYS_FSTAT, uintptr(fd), uintptr(_p0),
> uintptr(len(edir)))
>   	n = int(r0)
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -242,7 +216,6 @@
>   		_p0 = unsafe.Pointer(&_zero)
>   	}
>   	r0, _, e1 := Syscall(SYS_WSTAT,
> uintptr(unsafe.Pointer(StringBytePtr(path))), uintptr(_p0),
> uintptr(len(edir)))
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
> @@ -259,7 +232,6 @@
>   		_p0 = unsafe.Pointer(&_zero)
>   	}
>   	r0, _, e1 := Syscall(SYS_FWSTAT, uintptr(fd), uintptr(_p0),
> uintptr(len(edir)))
> -	err = nil
>   	if int(r0) == -1 {
>   		err = NewError(e1)
>   	}
>
>
>

[rsc, 2011-11-19 04:22:04]

very close, thanks

http://codereview.appspot.com/5411046/diff/3001/src/pkg/syscall/syscall_plan9.go
File src/pkg/syscall/syscall_plan9.go (right):

http://codereview.appspot.com/5411046/diff/3001/src/pkg/syscall/syscall_plan9...
src/pkg/syscall/syscall_plan9.go:41: func Syscall(trap, a1, a2, a3 uintptr) (r1,
r2 uintptr, err string)
Please change this to say err ErrorString,
along with the next three.

Then in the conversion program you can use err = e1 directly
instead of needing err = ErrorString(e1).
This is more in line with what the Unix ports do with Errno.

[lucio, 2011-11-19 05:09:47]

The second pair, Raw* use uintptr rather than error.  Can these still
be changed without harm?

Lucio.


On 11/19/11, rsc@golang.org <rsc@golang.org> wrote:
> very close, thanks
>
>
>
>
http://codereview.appspot.com/5411046/diff/3001/src/pkg/syscall/syscall_plan9.go
> File src/pkg/syscall/syscall_plan9.go (right):
>
>
http://codereview.appspot.com/5411046/diff/3001/src/pkg/syscall/syscall_plan9...
> src/pkg/syscall/syscall_plan9.go:41: func Syscall(trap, a1, a2, a3
> uintptr) (r1, r2 uintptr, err string)
> Please change this to say err ErrorString,
> along with the next three.
>
> Then in the conversion program you can use err = e1 directly
> instead of needing err = ErrorString(e1).
> This is more in line with what the Unix ports do with Errno.
>
> http://codereview.appspot.com/5411046/
>

[rsc, 2011-11-19 05:41:57]

On Sat, Nov 19, 2011 at 00:09, Lucio De Re <lucio.dere@gmail.com> wrote:
> The second pair, Raw* use uintptr rather than error.  Can these still
> be changed without harm?

no, you're right.  please leave those at uintptr.

[lucio, 2011-11-19 11:43:56]

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

Please take another look.

[lucio, 2011-11-19 15:21:18]

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

Please take another look.

[rsc, 2011-11-21 14:54:59]

LGTM

[rsc, 2011-11-21 14:55:18]

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

syscall: fix for Plan 9 build

exec_plan9.go:
. Adjusted return argument to match other changes.
#mksyscall.pl:
. Replaced "err = e1" with "err = NewError(e1)".
* Change abandoned, Russ made a better suggestion involving
  syscall_plan9.go.
syscall_plan9.go:
. Removed redundant "err = nil" lines.
. Adjusted //sys lines for mksyscall.pl.
* Replaced "err string" with "err ErrorString" in return arguments.
zsyscall_plan9_386.go:
. This module ought to be generated, but as it exists in the
  repository, I rebuilt it and checked that it matched expectations.
  Anybody is welcome to remove this from the repository if
  they feel it should go, but remember that not all Plan 9
  installations have a working Perl.

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

Committer: Russ Cox <rsc@golang.org>