Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: enable file creation w/ close-on-exec on darwin, freebsd #7187

Closed
mikioh opened this issue Jan 23, 2014 · 8 comments
Closed

os: enable file creation w/ close-on-exec on darwin, freebsd #7187

mikioh opened this issue Jan 23, 2014 · 8 comments
Milestone

Comments

@mikioh
Copy link
Contributor

@mikioh mikioh commented Jan 23, 2014

Both OS X 10.7 and FreeBSD 8 support O_CLOEXEC, so there's no reason to hesitate to
enable it on supported kernels.
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jan 23, 2014

Comment 1:

Hesitate?  I think nobody even cared to spend the time yet.
I only did Linux to reduce lock contention.
Feel free to add Darwin and FreeBSD support, but it shouldn't block Go 1.3.

Labels changed: added release-go1.3maybe, removed release-go1.3.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 23, 2014

Comment 2:

The os package already uses O_CLOEXEC on all Unix systems.  See os/file_unix.go.
I don't understand what change you are proposing.
@mikioh

This comment has been minimized.

Copy link
Contributor Author

@mikioh mikioh commented Jan 23, 2014

Comment 3:

> I don't understand what change you are proposing.
Something like the following.
--- a/src/pkg/syscall/zerrors_freebsd_amd64.go
+++ b/src/pkg/syscall/zerrors_freebsd_amd64.go
+   O_CLOEXEC                         = 0x100000
--- a/src/pkg/os/file_unix.go
+++ b/src/pkg/os/file_unix.go
+// supportsCloseOnExec reports whether the platform supports the
+// O_CLOEXEC flag.
+var supportsCloseOnExec bool
+
-   if syscall.O_CLOEXEC == 0 || runtime.GOOS == "darwin" { // O_CLOEXEC not supported
+   if !supportsCloseOnExec {
--- /dev/null
+++ b/src/pkg/os/sys_freebsd.go
+package os
+
+import "syscall"
+
+func init() {
+   // The O_CLOEXEC flag was introduced in FreeBSD 8.3.
+   if osrel, err := syscall.SysctlUint32("kern.osreldate"); err == nil {
+       // See http://www.freebsd.org/doc/en/books/porters-handbook/freebsd-versions.html.
+       if osrel >= 803000 {
+           supportsCloseOnExec = true
+       }
+   }
+}
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 23, 2014

Comment 4:

What happens if you pass O_CLOEXEC on FreeBSD versions before 8.2?  Can we check for
some appropriate error and retry without O_CLOEXEC, the way we handle SOCK_CLOEXEC?
@mikioh

This comment has been minimized.

Copy link
Contributor Author

@mikioh mikioh commented Jan 23, 2014

Comment 5:

Or...
if supportsCloseOnExec {
        flag |= syscall.O_CLOEXEC
}
r, e := syscall.Open(name, flag, syscallMode(perm))
@mikioh

This comment has been minimized.

Copy link
Contributor Author

@mikioh mikioh commented Jan 23, 2014

Comment 6:

> the way we handle SOCK_CLOEXEC?
Not sure whether such simple fallback works well, but will have a look at kernel code
tomorrow. Hm, probably kernel just ignores unsupported flags because,
./zerrors_darwin_386.go:    O_CLOEXEC                         = 0x1000000
./zerrors_darwin_amd64.go:  O_CLOEXEC                         = 0x1000000
but
if syscall.O_CLOEXEC == 0 || runtime.GOOS == "darwin" { // O_CLOEXEC not supported
sigh. According to https://code.google.com/p/go-wiki/wiki/DashboardBuilders, OS X 10.6
which includes BSD subsystem comes from FreeBSD shows us no error until now.
@mikioh

This comment has been minimized.

Copy link
Contributor Author

@mikioh mikioh commented Jan 24, 2014

Comment 7:

Confirmed, unlike IPC stuff such as socket or mmap, O_CLOEXEC is just ignored, sigh.
See fallocf in 8/9-STABLE or finstall in 10-STABLE.
http://svnweb.freebsd.org/base?view=revision&revision=220241
@mikioh

This comment has been minimized.

Copy link
Contributor Author

@mikioh mikioh commented Mar 4, 2014

Comment 8:

This issue was closed by revision be8aa4b.

Status changed to Fixed.

@mikioh mikioh added fixed labels Mar 4, 2014
@rsc rsc added this to the Go1.3 milestone Apr 14, 2015
@rsc rsc removed the release-go1.3maybe label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.