code review 64510043: os: handle file creation with close-on-exec flag correctly on darwin, freebsd

os: handle file creation with close-on-exec flag correctly on darwin, freebsd

Fixes issue 7187.
Update issue 7193

[mikio, 2014-02-26 06:54:43]

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

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

[dfc, 2014-02-26 07:49:38]

Thanks for taking a look at this. Is it possible to do this via a constant on
platforms that we know support close on exec unconditionally ?

https://codereview.appspot.com/64510043/diff/140001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/64510043/diff/140001/src/pkg/os/file_unix.go#n...
src/pkg/os/file_unix.go:73: var supportsCloseOnExec bool
Is it possible to make this a constant on some platforms?

https://codereview.appspot.com/64510043/diff/140001/src/pkg/os/file_unix.go#n...
src/pkg/os/file_unix.go:88: if !supportsCloseOnExec {
then this will compile down to nothing if supportsCloseOnExec is true

https://codereview.appspot.com/64510043/diff/140001/src/pkg/os/sys_darwin.go
File src/pkg/os/sys_darwin.go (right):

https://codereview.appspot.com/64510043/diff/140001/src/pkg/os/sys_darwin.go#...
src/pkg/os/sys_darwin.go:7: import "syscall"

add

var supportsCloseOnExec bool

https://codereview.appspot.com/64510043/diff/140001/src/pkg/os/sys_freebsd.go
File src/pkg/os/sys_freebsd.go (right):

https://codereview.appspot.com/64510043/diff/140001/src/pkg/os/sys_freebsd.go...
src/pkg/os/sys_freebsd.go:7: import "syscall"
same

https://codereview.appspot.com/64510043/diff/140001/src/pkg/os/sys_unix.go
File src/pkg/os/sys_unix.go (right):

https://codereview.appspot.com/64510043/diff/140001/src/pkg/os/sys_unix.go#ne...
src/pkg/os/sys_unix.go:8: 
drop the init() and add

const supportsCloseOnExec = true

[mikio, 2014-02-26 07:52:10]

> Thanks for taking a look at this. Is it possible to do this via a constant on
> platforms that we know support close on exec unconditionally ?

how? i don't think it's possible except dropping support for old versions.

[dfc, 2014-02-26 07:55:16]

Have a look at my comments, hopefully they make sense.


On Wed, Feb 26, 2014 at 6:52 PM, <mikioh.mikioh@gmail.com> wrote:

> Thanks for taking a look at this. Is it possible to do this via a
>>
> constant on
>
>> platforms that we know support close on exec unconditionally ?
>>
>
> how? i don't think it's possible except dropping support for old
> versions.
>
> https://codereview.appspot.com/64510043/
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[mikio, 2014-02-26 07:56:59]

On 2014/02/26 07:55:16, dfc wrote:
> Have a look at my comments, hopefully they make sense.

i don't understand why such optimization so important to you.

[dfc, 2014-02-26 07:57:50]

Anything we can push into the compiler makes the runtime start faster, less
init() functions, and make the code smaller.


On Wed, Feb 26, 2014 at 6:57 PM, <mikioh.mikioh@gmail.com> wrote:

> On 2014/02/26 07:55:16, dfc wrote:
>
>> Have a look at my comments, hopefully they make sense.
>>
>
> i don't understand why such optimization so important to you.
>
> https://codereview.appspot.com/64510043/
>

[mikio, 2014-02-26 08:06:09]

> Anything we can push into the compiler makes the runtime start faster, less
> init() functions, and make the code smaller.

anything? hm, i`ll check your contributed code later. :)
fair enough, will do so.

[dfc, 2014-02-26 08:11:33]

>
> anything? hm, i`ll check your contributed code later. :)
>

Heh. Well, anything that we can reasonably turn into a constant that can be
evaluated at compile time rather than a variable that has to be evaluated
at runtime is something I am very keen to pursue. If you know of cases
where i've not lived up to my own standards, please let me know.


> fair enough, will do so.
>
>
Thanks.


> https://codereview.appspot.com/64510043/
>

[mikio, 2014-02-26 11:25:29]

Hello golang-codereviews@googlegroups.com, dave@cheney.net (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[mikio, 2014-02-27 15:50:24]

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

Please take another look.

[bradfitz, 2014-02-27 22:25:30]

LGTM but add support for the "nacl" GOOS first, otherwise it won't compile.

[mikio, 2014-02-28 01:20:56]

wait for nacl landing. also added sys_nacl.go and solaris tag to sys_unix.go

[dfc, 2014-02-28 02:15:03]

I'm fine with you submitting this now. I'm pretty sure nacl isn't going to work
just yet and it's trivial to fix for this case. 

Also, as you can't reasonable test nacl at the moment, you should let that block
you. 

Adding a TODO somewhere would be more that sufficient. 

> On 28 Feb 2014, at 12:20, mikioh.mikioh@gmail.com wrote:
> 
> wait for nacl landing. also added sys_nacl.go and solaris tag to
> sys_unix.go
> 
> https://codereview.appspot.com/64510043/

[rsc, 2014-02-28 02:22:33]

Please feel free to break NaCl as you see fit, until there is a builder
that says "ok".
That's true of any port.

[mikio, 2014-02-28 02:47:13]

thanks, but i realised that this cl alos requires freebsd10 cls.

On Fri, Feb 28, 2014 at 11:22 AM, Russ Cox <rsc@golang.org> wrote:
> Please feel free to break NaCl as you see fit, until there is a builder that
> says "ok".
> That's true of any port.
>

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

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

os: handle file creation with close-on-exec flag correctly on darwin, freebsd

Fixes issue 7187.
Update issue 7193

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

[gobot, 2014-03-04 04:32:23]

This CL appears to have broken the linux-amd64-race builder.