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

unix: simplify uv__cloexec_fcntl() #3492

Merged
merged 1 commit into from Mar 5, 2022

Conversation

bnoordhuis
Copy link
Member

FD_CLOEXEC is the only defined flag for fcntl(F_SETFD) so don't bother
getting the status of that flag first with fcntl(F_GETFD), just set it.

cc @vtjnash - as mentioned in #3490

FD_CLOEXEC is the only defined flag for fcntl(F_SETFD) so don't bother
getting the status of that flag first with fcntl(F_GETFD), just set it.
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible some platform defines other bits and would be broken by this?

@bnoordhuis
Copy link
Member Author

Not to my knowledge and I think it's highly unlikely. FD_CLOEXEC being the sole flag is one of those bits of UNIX lore that's been around so long it's entrenched.

@vtjnash
Copy link
Member

vtjnash commented Mar 1, 2022

True, but we also pretty much only use this code path on weird platforms (e.g. not AIX, Apple, other BSDs, or linux)

@bnoordhuis
Copy link
Member Author

I suppose so1 and I don't feel too strongly but this set-and-forget idiom is widespread enough (random example: nginx) that I'm confident it's safe, even on obscure platforms.

1 As of #3257 it's called directly from src/unix/process.c for reasons I'm not wholly clear on.

@vtjnash
Copy link
Member

vtjnash commented Mar 4, 2022

It is from #1167, since calling ioctl is not required to be safe to call after fork. (see man 7 signal-safety and man 2 fork). But also the only reason we use the ioctl is to avoid the need for 2 syscalls here. StackOverflow seems to think that fcntl might be a better choice always, and we can also now eliminate the ioctl (https://stackoverflow.com/a/1151077/1712368) for this. See also #1832.

@vtjnash vtjnash merged commit c8583bb into libuv:v1.x Mar 5, 2022
vtjnash added a commit to vtjnash/libuv that referenced this pull request Mar 5, 2022
Now that uv__cloexec_fcntl() is simplified
(libuv#3492), there is no benefit to
maintaining duplicate code paths for the same thing.
vtjnash added a commit that referenced this pull request Mar 6, 2022
Now that uv__cloexec_fcntl() is simplified
(#3492), there is no benefit to
maintaining duplicate code paths for the same thing.
JeffroMF pushed a commit to JeffroMF/libuv that referenced this pull request May 16, 2022
FD_CLOEXEC is the only defined flag for fcntl(F_SETFD) so don't bother
getting the status of that flag first with fcntl(F_GETFD), just set it.
JeffroMF pushed a commit to JeffroMF/libuv that referenced this pull request May 16, 2022
Now that uv__cloexec_fcntl() is simplified
(libuv#3492), there is no benefit to
maintaining duplicate code paths for the same thing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants