-
Notifications
You must be signed in to change notification settings - Fork 3k
Add support for POSIX O_CLOFORK #1698
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
Are you sure you want to change the base?
Conversation
Skimming the thread, the main objection seems to be that the feature adds extra memory accesses to certain hot paths. I think this is not so true in our case, we already pay some of these overheads. We have 7 bits available for per-fd flags (struct filedescent is pretty fat anyway since it stores capsicum rights), and we already have to iterate over fds upon fork since kqueue descriptors implicitly have O_CLOFORK set. XNU seems to implement this flag as well. I don't have strong feelings on whether it's very useful or not, but at a glance the implementation doesn't look too complex. |
I do share the sentiment that the feature is silly. But the implementation should be straighforward, since the main pain point is fdcopy() where we already do the pass over all fds in the parent. IMO it should be finished if only to have a checkpoint for POSIX compliance. |
2a306bf
to
267133c
Compare
6102aa1
to
2fb5caa
Compare
CDDL-licensed code from illumos should be placed under cddl/contrib/opensolaris. It's ok for the build outputs to be installed under /usr/tests, but the code should be excluded from the build if |
2854c77
to
77c1f74
Compare
The build is not done automatically. To test:
|
a3d476d
to
7aeab4f
Compare
The
|
7aeab4f
to
a7344b7
Compare
oclo tests results all PASSED:
|
Why not build it automatically? If it's not connected to the build, we won't catch regressions easily. |
a7344b7
to
f909915
Compare
It now builds but doesn't run automatically because |
f909915
to
1d489e3
Compare
I just used I see the |
kyua is a test runner that can ingest outputs from different testing protocols (TAP, ATF, googletest, ...). It looks like oclo just prints results to stdout and uses its exit status to indicate whether all tests passed or not? If so, I think you can try building it with |
1d489e3
to
da66573
Compare
That worked out beautifully. I'm beginning to like kyua. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is basically the thing that I very much hate about dup3(), you get 2^n ops when you start adding flags.
I do not think this part is sustainable. Some scalable design is needed there.
A possible solution is to define some new fcntl op with a code, say F_DUP3 23. Then, if none of the direct comparision of fcntl op with existing ops succeeds, we mask out top 16 bits from ops and see if result is eq to F_DUP3. If it is, then top 16 bits are interpreted as flags, and we put there CLOEXEC, your new CLOFORK, and have 14 more bits to tweak fd flags if any are grown.
aa83673
to
3b9c824
Compare
I implemented your proposal, which is better than using CLOBOTH. All the tests are passing. The only downside is that the semantics of |
3b9c824
to
599cac4
Compare
Our own tests also pass:
|
@@ -280,6 +280,7 @@ typedef __pid_t pid_t; | |||
#endif | |||
#if __BSD_VISIBLE | |||
#define F_DUP2FD_CLOFORK 24 /* Like F_DUP2FD, but FD_CLOFORK is set */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we still need this command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we still need this command?
To have the close-on-fork equivalent of F_DUP2FD_CLOEXEC, as Illumos does.
599cac4
to
199decc
Compare
Rebased to take uexterr_gettext into account. Can we merge? |
This PR:
This one looks perhaps deceptively simple to implement but it's passing all 667 tests (except
dup3
) from a suite adapted from Illumos not included yet.This may be useful for Golang & Rust:
Command::spawn
on a newly-written file can fail with ETXTBSY due to racing with itself on Unix rust-lang/rust#114554This interface was called stupid by opinionated Linux folks and perhaps they're right, so I want an informed opinion before going any further. I had my fun anyway. Here are the relevant links with discussion:
DONE
dup3
NOT NEEDED
"f"
infopen
. It's not POSIX and not even Solaris/Illumos implement it.NOTES
POSIX References:
Illumos references:
Fixes https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=286843
To test: