code review 12476043: net: use F_DUPFD_CLOEXEC when duping fds

net: use F_DUPFD_CLOEXEC when duping fds

This means that in the common case (modern kernel), we only
make 1 system call to dup instead of two, and we also avoid
grabbing the syscall.ForkLock.

[bradfitz, 2013-08-05 20:16:22]

Hello golang-dev@googlegroups.com,

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

[iant, 2013-08-05 20:40:55]

https://codereview.appspot.com/12476043/diff/3001/src/pkg/net/fd_linux.go
File src/pkg/net/fd_linux.go (right):

https://codereview.appspot.com/12476043/diff/3001/src/pkg/net/fd_linux.go#new...
src/pkg/net/fd_linux.go:1: // Copyright 2013 The Go Authors. All rights
reserved.
F_DUPFD_CLOEXEC is in POSIX.1-2008
(http://pubs.opengroup.org/onlinepubs/9699919799/) and as such also exists on at
least Darwin, NetBSD, OpenBSD, and FreeBSD (although for some reason it is not
defined in our syscall package for FreeBSD i386 and amd64).  So this shouldn't
be in fd_linux.go.  In fact if we fix the syscall package for FreeBSD, it seems
that it could go into a _unix.go file.

https://codereview.appspot.com/12476043/diff/3001/src/pkg/net/fd_linux.go#new...
src/pkg/net/fd_linux.go:29: return 0, e1
Perhaps s/0/-1/ , for better behaviour for poorly written programs.

[bradfitz, 2013-08-05 21:41:49]

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

Please take another look.

[bradfitz, 2013-08-05 21:42:02]

PTAL

Oh, nice.  Done.



On Mon, Aug 5, 2013 at 1:40 PM, <iant@golang.org> wrote:

>
> https://codereview.appspot.**com/12476043/diff/3001/src/**
>
pkg/net/fd_linux.go<https://codereview.appspot.com/12476043/diff/3001/src/pkg/net/fd_linux.go>
> File src/pkg/net/fd_linux.go (right):
>
> https://codereview.appspot.**com/12476043/diff/3001/src/**
>
pkg/net/fd_linux.go#newcode1<https://codereview.appspot.com/12476043/diff/3001/src/pkg/net/fd_linux.go#newcode1>
> src/pkg/net/fd_linux.go:1: // Copyright 2013 The Go Authors. All rights
> reserved.
> F_DUPFD_CLOEXEC is in POSIX.1-2008
>
(http://pubs.opengroup.org/**onlinepubs/9699919799/<http://pubs.opengroup.org/...)
> and as such also
> exists on at least Darwin, NetBSD, OpenBSD, and FreeBSD (although for
> some reason it is not defined in our syscall package for FreeBSD i386
> and amd64).  So this shouldn't be in fd_linux.go.  In fact if we fix the
> syscall package for FreeBSD, it seems that it could go into a _unix.go
> file.
>
> https://codereview.appspot.**com/12476043/diff/3001/src/**
>
pkg/net/fd_linux.go#newcode29<https://codereview.appspot.com/12476043/diff/3001/src/pkg/net/fd_linux.go#newcode29>
> src/pkg/net/fd_linux.go:29: return 0, e1
> Perhaps s/0/-1/ , for better behaviour for poorly written programs.
>
>
https://codereview.appspot.**com/12476043/<https://codereview.appspot.com/124...
>

[bradfitz, 2013-08-05 22:00:24]

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

Please take another look.

[iant, 2013-08-05 22:39:34]

LGTM

[bradfitz, 2013-08-05 22:43:48]

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

net: use F_DUPFD_CLOEXEC when duping fds

This means that in the common case (modern kernel), we only
make 1 system call to dup instead of two, and we also avoid
grabbing the syscall.ForkLock.

R=golang-dev, iant
CC=golang-dev
https://codereview.appspot.com/12476043

[dave_cheney.net, 2013-08-06 00:17:37]

On 2013/08/05 22:43:48, bradfitz wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=4b59ea60560f ***
> 
> net: use F_DUPFD_CLOEXEC when duping fds
> 
> This means that in the common case (modern kernel), we only
> make 1 system call to dup instead of two, and we also avoid
> grabbing the syscall.ForkLock.
> 
> R=golang-dev, iant
> CC=golang-dev
> https://codereview.appspot.com/12476043

Bad news. This broken OS X 10.6.
http://build.golang.org/log/4189569958ff4bea7543f302ad35e1420ede69ee. Maybe
there is a bug with the fallback logic.

[bradfitz, 2013-08-06 00:18:36]

On Mon, Aug 5, 2013 at 5:17 PM, <dave@cheney.net> wrote:

> On 2013/08/05 22:43:48, bradfitz wrote:
>
>> *** Submitted as
>>
>
https://code.google.com/p/go/**source/detail?r=4b59ea60560f<https://code.goog...
>
>  net: use F_DUPFD_CLOEXEC when duping fds
>>
>
>  This means that in the common case (modern kernel), we only
>> make 1 system call to dup instead of two, and we also avoid
>> grabbing the syscall.ForkLock.
>>
>
>  R=golang-dev, iant
>> CC=golang-dev
>>
https://codereview.appspot.**com/12476043<https://codereview.appspot.com/1247...
>>
>
> Bad news. This broken OS X 10.6.
>
http://build.golang.org/log/**4189569958ff4bea7543f302ad35e1**420ede69ee<http...
> .
> Maybe there is a bug with the fallback logic.
>

I discovered that 14 minutes ago and sent out a CL which you could LGTM.
 :-)