code review 68880043: syscall: add Accept4 for freebsd

syscall: add Accept4 for freebsd

Update issue 7186
Update issue 7428

[mikio, 2014-02-27 15:46:32]

Hello golang-codereviews@googlegroups.com, rsc@golang.org, minux.ma@gmail.com
(cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[r, 2014-02-27 16:52:11]

LGTM

[r, 2014-02-27 16:53:57]

please address comments; don't submit yet

https://codereview.appspot.com/68880043/diff/360001/src/pkg/syscall/syscall_f...
File src/pkg/syscall/syscall_freebsd.go (right):

https://codereview.appspot.com/68880043/diff/360001/src/pkg/syscall/syscall_f...
src/pkg/syscall/syscall_freebsd.go:106: func Accept4(fd, flags int) (nfd int, sa
Sockaddr, err error) {
actually maybe add a comment here, because it's a bit odd to have Accept4 take
two arguments.
also, why is len passed as a pointer? is its value important to the caller?

[bradfitz, 2014-02-27 17:12:27]

LGTM

https://codereview.appspot.com/68880043/diff/360001/src/pkg/syscall/syscall_f...
File src/pkg/syscall/syscall_freebsd.go (right):

https://codereview.appspot.com/68880043/diff/360001/src/pkg/syscall/syscall_f...
src/pkg/syscall/syscall_freebsd.go:106: func Accept4(fd, flags int) (nfd int, sa
Sockaddr, err error) {
On 2014/02/27 16:53:58, r wrote:
> actually maybe add a comment here, because it's a bit odd to have Accept4 take
> two arguments.

That's the point of Accept4 over Accept, to add flags.

> also, why is len passed as a pointer? is its value important to the caller?

It's a value+result parameter.  See the man page.  That's why this wrapper
(capital Accept4) exists, to give it a more Go-feeling API, rather than the
lowercase C-like accept4.

This is the same as the Linux version:  http://golang.org/pkg/syscall/#Accept4

[r, 2014-02-27 21:40:10]

Brad, if you're happy with this just LGTM it. I'm not arguing it shouldn't go
in, I'm just trying to understand another horrible piece of network API. Perhaps
i shouldn't even try.

https://codereview.appspot.com/68880043/diff/360001/src/pkg/syscall/syscall_f...
File src/pkg/syscall/syscall_freebsd.go (right):

https://codereview.appspot.com/68880043/diff/360001/src/pkg/syscall/syscall_f...
src/pkg/syscall/syscall_freebsd.go:106: func Accept4(fd, flags int) (nfd int, sa
Sockaddr, err error) {
I understand that, but that's not on point. My question is: if len is in the C
api why isn't it in the Go api? To put it another way, if it's a value+result
parameter, why isn't it checked after the system call?

[bradfitz, 2014-02-27 21:47:52]

https://codereview.appspot.com/68880043/diff/360001/src/pkg/syscall/syscall_f...
File src/pkg/syscall/syscall_freebsd.go (right):

https://codereview.appspot.com/68880043/diff/360001/src/pkg/syscall/syscall_f...
src/pkg/syscall/syscall_freebsd.go:106: func Accept4(fd, flags int) (nfd int, sa
Sockaddr, err error) {
On 2014/02/27 21:40:10, r wrote:
> I understand that, but that's not on point. My question is: if len is in the C
> api why isn't it in the Go api? To put

 it another way, if it's a value+result
> parameter, why isn't it checked after the system call?

That's a good question. Maybe we don't need it because func anyToSockaddr in
syscall_linux.go etc switches on rsa.Addr.Family and then knows its size.

Or maybe we've just always gotten lucky.

I don't know the history of that code. Maybe Ian does.

But this Darwin code is just adding the accept4 system call, identical in API
and code to other Unix systems like Linux, so we can pass the O_CLOEXEC flag and
save some locking + system call later.

Filed https://code.google.com/p/go/issues/detail?id=7428 to investigate.

[mikio, 2014-02-27 22:00:25]

On Fri, Feb 28, 2014 at 1:53 AM,  <r@golang.org> wrote:

> please address comments; don't submit yet

no worries, this cl requires freebsd10 cls.

> actually maybe add a comment here, because it's a bit odd to have
> Accept4 take two arguments.

for the SOCK_CLOEXEC flag, yay!

> also, why is len passed as a pointer? is its value important to the
> caller?

len is the last resort to determine its address family. it doesn't
happen for consumer sockets recently, though.
fyi: poor routing sockets:
https://code.google.com/p/go/source/browse/src/pkg/syscall/route_bsd.go#144

[r, 2014-02-27 22:10:52]

LGTM

[mikio, 2014-02-28 00:37:47]

added a panic the same as linux, for  issue 7428. it would be useful when we
develop IPv10; carrying data labeled IPv4 or IPv6 address family using IPv10
protocol family transport. look forward to see that happen.

[mikio, 2014-03-04 00:27:54]

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

syscall: add Accept4 for freebsd

Update issue 7186
Update issue 7428

LGTM=r, bradfitz
R=golang-codereviews, rsc, minux.ma, r, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/68880043

[gobot, 2014-03-04 02:03:57]

This CL appears to have broken the windows-386-ec2 builder.