code review 5491050: syscall: Changes to the syscall package to support NetBSD.

syscall: Changes to the syscall package to support NetBSD.

Not all syscalls are implemented, but many are. On the suggestion
of Joel Sing <jsing@google.com>, the generated files were added
with hg add instead of hg cp, since they are generated on an OS
dependant basis.

[m4dh4tt3r, 2011-12-15 06:42:05]

Hello golang-dev@googlegroups.com,

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

[jsing, 2011-12-15 15:22:03]

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

http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mkall.sh#newcode176
src/pkg/syscall/mkall.sh:176: mksysnum="curl -s
'http://cvsweb.netbsd.org/bsdweb.cgi/~checkout~/src/sys/kern/syscalls.master?rev=1.211&content-type=text/plain&only_with_tag=netbsd-5-1'
| ./mksysnum_netbsd.pl"
With FreeBSD and OpenBSD, we pull the current syscalls.master rather than a
specific release/revision. We should probably be consistent with NetBSD.

http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mksyscall.pl
File src/pkg/syscall/mksyscall.pl (right):

http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mksyscall.pl#new...
src/pkg/syscall/mksyscall.pl:137: } elsif($type eq "int64" && $openbsd) {
Since the code/purpose is the same we could make this ($openbsd || $netbsd)
instead.

http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mksysctl_netbsd.pl
File src/pkg/syscall/mksysctl_netbsd.pl (right):

http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mksysctl_netbsd....
src/pkg/syscall/mksysctl_netbsd.pl:1: #!/usr/pkg/bin/perl
Be nice to keep this consistent...

http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mksysctl_netbsd....
src/pkg/syscall/mksysctl_netbsd.pl:8: # Parse the header files for NetBSD and
generate a Go usable sysctl MIB.
From what I recall this should not be necessary for NetBSD, since the MIB
information can be provided by the kernel (although I do not recall the
specifics - see lib/libc/gen/sysctlnametomib.c).

http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/ztypes_netbsd_38...
File src/pkg/syscall/ztypes_netbsd_386.go (right):

http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/ztypes_netbsd_38...
src/pkg/syscall/ztypes_netbsd_386.go:2: // cgo -godefs types_openbsd.go
types_openbsd.go? Was this generated from the wrong file (or not regenerated)?

http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/ztypes_netbsd_am...
File src/pkg/syscall/ztypes_netbsd_amd64.go (right):

http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/ztypes_netbsd_am...
src/pkg/syscall/ztypes_netbsd_amd64.go:2: // cgo -godefs types_openbsd.go
types_openbsd.go? was this generated from the wrong file (or not regenerated)?

[m4dh4tt3r, 2011-12-15 21:14:45]

PTAL

On 2011/12/15 15:22:03, jsing wrote:
> http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mkall.sh
> File src/pkg/syscall/mkall.sh (right):
> 
>
http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mkall.sh#newcode176
> src/pkg/syscall/mkall.sh:176: mksysnum="curl -s
>
'http://cvsweb.netbsd.org/bsdweb.cgi/~checkout~/src/sys/kern/syscalls.master?rev=1.211&content-type=text/plain&only_with_tag=netbsd-5-1'
> | ./mksysnum_netbsd.pl"
> With FreeBSD and OpenBSD, we pull the current syscalls.master rather than a
> specific release/revision. We should probably be consistent with NetBSD.

I did it this way for simplicity. The current syscalls.master has a very
different format than the other BSDs, and I'll need to significantly rework
mksysnum_netbsd.pl. Can we leave this for now, and I will submit another CL that
uses current syscalls.master?

> http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mksyscall.pl
> File src/pkg/syscall/mksyscall.pl (right):
> 
>
http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mksyscall.pl#new...
> src/pkg/syscall/mksyscall.pl:137: } elsif($type eq "int64" && $openbsd) {
> Since the code/purpose is the same we could make this ($openbsd || $netbsd)
> instead.

Done.

>
http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mksysctl_netbsd.pl
> File src/pkg/syscall/mksysctl_netbsd.pl (right):
> 
>
http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mksysctl_netbsd....
> src/pkg/syscall/mksysctl_netbsd.pl:1: #!/usr/pkg/bin/perl
> Be nice to keep this consistent...

Oops. Snuck in by mistake.
Done.

>
http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/mksysctl_netbsd....
> src/pkg/syscall/mksysctl_netbsd.pl:8: # Parse the header files for NetBSD and
> generate a Go usable sysctl MIB.
> From what I recall this should not be necessary for NetBSD, since the MIB
> information can be provided by the kernel (although I do not recall the
> specifics - see lib/libc/gen/sysctlnametomib.c).

From my read of the code, you are correct. Removed the script, generated file,
and the file generation from mkall.sh.

>
http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/ztypes_netbsd_38...
> File src/pkg/syscall/ztypes_netbsd_386.go (right):
> 
>
http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/ztypes_netbsd_38...
> src/pkg/syscall/ztypes_netbsd_386.go:2: // cgo -godefs types_openbsd.go
> types_openbsd.go? Was this generated from the wrong file (or not regenerated)?

Another oops.
Done.
 
>
http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/ztypes_netbsd_am...
> File src/pkg/syscall/ztypes_netbsd_amd64.go (right):
> 
>
http://codereview.appspot.com/5491050/diff/1/src/pkg/syscall/ztypes_netbsd_am...
> src/pkg/syscall/ztypes_netbsd_amd64.go:2: // cgo -godefs types_openbsd.go
> types_openbsd.go? was this generated from the wrong file (or not regenerated)?

Same as above.

[jsing, 2011-12-16 03:51:07]

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

http://codereview.appspot.com/5491050/diff/5003/src/pkg/syscall/mkall.sh#newc...
src/pkg/syscall/mkall.sh:183: *)
This case has been duplicated from below - please remove.

http://codereview.appspot.com/5491050/diff/5003/src/pkg/syscall/mksysnum_netb...
File src/pkg/syscall/mksysnum_netbsd.pl (right):

http://codereview.appspot.com/5491050/diff/5003/src/pkg/syscall/mksysnum_netb...
src/pkg/syscall/mksysnum_netbsd.pl:23: if(/^([0-9]+)\s+STD\s+(RUMP\s+)?({
\S+\s+\*?(\w+).*)$/){
Changing this to:

/^([0-9]+)\s+STD\s+(RUMP\s+)?({\s*\S+\s*\*?\|\S+\|\S*\|(\w+).*)$/

Should make it work with the new syscalls.master format.

[jsing, 2011-12-16 03:55:59]

Re the change to the syscalls.master format, I notice you've changed the
mkall.sh script, but not the mksysnum_netbsd.pl - I think the change is simple
enough (with the regexp I provided) to include this now, but I'll let you make
the final call.

Looking good...

http://codereview.appspot.com/5491050/diff/5003/src/pkg/syscall/syscall_netbs...
File src/pkg/syscall/syscall_netbsd.go (right):

http://codereview.appspot.com/5491050/diff/5003/src/pkg/syscall/syscall_netbs...
src/pkg/syscall/syscall_netbsd.go:33: func nametomib(name string) (mib []_C_int,
err error) {
This will fail since there is no sysctlMib - please stub out for now (just
leaving the last line).

[m4dh4tt3r, 2011-12-16 22:07:45]

PTAL

On 2011/12/16 03:55:59, jsing wrote:
> Re the change to the syscalls.master format, I notice you've changed the
> mkall.sh script, but not the mksysnum_netbsd.pl - I think the change is simple
> enough (with the regexp I provided) to include this now, but I'll let you make
> the final call.

I added the mksysnum_netbsd.pl that you provided and regenerated the files.

> Looking good...

Thanks for the review and the help.

>
http://codereview.appspot.com/5491050/diff/5003/src/pkg/syscall/syscall_netbs...
> File src/pkg/syscall/syscall_netbsd.go (right):
> 
>
http://codereview.appspot.com/5491050/diff/5003/src/pkg/syscall/syscall_netbs...
> src/pkg/syscall/syscall_netbsd.go:33: func nametomib(name string) (mib
[]_C_int,
> err error) {
> This will fail since there is no sysctlMib - please stub out for now (just
> leaving the last line).

Done.

[mikio, 2011-12-17 04:41:35]

can you pls update both zsysnum_netbsd_{386,amd64}.go.
looks like SYS_SOCKET, others are missing.

http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/syscall_netbs...
File src/pkg/syscall/syscall_netbsd_amd64.go (right):

http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/syscall_netbs...
src/pkg/syscall/syscall_netbsd_amd64.go:27: k.Ident = uint32(fd)
s/32/64/

[jsing, 2011-12-17 12:53:52]

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

http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/mkall.sh#newc...
src/pkg/syscall/mkall.sh:209: darwin | freebsd | openbsd)
You need to add netbsd to this list.

http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/ztypes_netbsd...
File src/pkg/syscall/ztypes_netbsd_386.go (right):

http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/ztypes_netbsd...
src/pkg/syscall/ztypes_netbsd_386.go:2: // cgo -godefs types_openbsd.go
types_openbsd.go again? :)

http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/ztypes_netbsd...
File src/pkg/syscall/ztypes_netbsd_amd64.go (right):

http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/ztypes_netbsd...
src/pkg/syscall/ztypes_netbsd_amd64.go:2: // cgo -godefs types_openbsd.go
Same...

[jsing, 2011-12-17 14:31:31]

Please also remove the zsysctl_netbsd.go entry from the GOFILES_netbsd list in
src/pkg/syscall/Makefile.

[m4dh4tt3r, 2011-12-17 18:45:02]

On 2011/12/17 12:53:52, jsing wrote:
> http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/mkall.sh
> File src/pkg/syscall/mkall.sh (right):
> 
>
http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/mkall.sh#newc...
> src/pkg/syscall/mkall.sh:209: darwin | freebsd | openbsd)
> You need to add netbsd to this list.

Done.

>
http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/ztypes_netbsd...
> File src/pkg/syscall/ztypes_netbsd_386.go (right):
> 
>
http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/ztypes_netbsd...
> src/pkg/syscall/ztypes_netbsd_386.go:2: // cgo -godefs types_openbsd.go
> types_openbsd.go again? :)

Done. :-)

>
http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/ztypes_netbsd...
> File src/pkg/syscall/ztypes_netbsd_amd64.go (right):
> 
>
http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/ztypes_netbsd...
> src/pkg/syscall/ztypes_netbsd_amd64.go:2: // cgo -godefs types_openbsd.go
> Same...

Done.

[m4dh4tt3r, 2011-12-17 18:45:15]

On 2011/12/17 14:31:31, jsing wrote:
> Please also remove the zsysctl_netbsd.go entry from the GOFILES_netbsd list in
> src/pkg/syscall/Makefile.

Done.

[m4dh4tt3r, 2011-12-17 18:45:33]

On 2011/12/17 04:41:35, mikioh wrote:
> can you pls update both zsysnum_netbsd_{386,amd64}.go.
> looks like SYS_SOCKET, others are missing.

Done.

>
http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/syscall_netbs...
> File src/pkg/syscall/syscall_netbsd_amd64.go (right):
> 
>
http://codereview.appspot.com/5491050/diff/2053/src/pkg/syscall/syscall_netbs...
> src/pkg/syscall/syscall_netbsd_amd64.go:27: k.Ident = uint32(fd)
> s/32/64/

[jsing, 2011-12-19 16:51:44]

LGTM

There are still a few issues lurking in here, however we can fix these in
separate (smaller) changes.

[jsing, 2011-12-19 16:58:13]

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

syscall: Changes to the syscall package to support NetBSD.

Not all syscalls are implemented, but many are. On the suggestion
of Joel Sing <jsing@google.com>, the generated files were added
with hg add instead of hg cp, since they are generated on an OS
dependant basis.

R=golang-dev, jsing, mikioh.mikioh
CC=golang-dev
http://codereview.appspot.com/5491050

Committer: Joel Sing <jsing@google.com>