-
Notifications
You must be signed in to change notification settings - Fork 385
Support F_GETFL and F_SETFL for fcntl #4212
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
base: master
Are you sure you want to change the base?
Conversation
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.
@rustbot ready
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.
Here's the first round of comments. Sorry for the long wait!
@rustbot author
src/shims/unix/fd.rs
Outdated
if flag == this.eval_libc_i32("O_NONBLOCK") { | ||
anonsocket_fd.set_nonblock(); | ||
} else { | ||
throw_unsup_format!("fcntl: only O_NONBLOCK are supported for F_GETFL") | ||
} |
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 should also support un-setting the O_NONBLOCK
flag, right? I assume that's what happens on flag == 0
.
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.
Good call!
I just realised this is indeed how tokio set their fd from non-blocking to blocking. https://github.com/tokio-rs/tokio/blob/817fa605ee6a2549fe8e6057ec23a8309d42d2e9/tokio/src/net/unix/pipe.rs#L1432
Reminder, once the PR becomes ready for a review, use |
Co-authored-by: Ralf Jung <[email protected]>
Co-authored-by: Ralf Jung <[email protected]>
On the latest commit b22019d, I made all the tests in this PR to use isolation mode. |
err_unsup_format!("fcntl: only socketpair / pipe are supported for F_GETFL") | ||
})?; | ||
|
||
if anonsocket_fd.is_nonblock() { |
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.
Does this match real implementations -- only returning O_NONBLOCK and no other flag?
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.
I overlooked something, we need to return O_RDWR
for socketpair
, O_RDONLY
and O_WRONLY
for pipe
. So we need to add something to differentiate between socketpair
and pipe
's reading / writing end here.
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.
Probably we should have get_flags
and set_flags
methods on the FD trait so this logic can live with the FD implementation.
When a socketpair's peer FD has been closed, does it become RDONLY?
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.
When a socketpair's peer FD has been closed, does it become RDONLY?
fcntl(F_GETFL)
will still return O_RDWR
even though the subsequent write will error with EPIPE
.
Same thing happened to pipe
too. So both socketpair
and pipe
fds have fixed O_RDONLY
/ O_WRONLY
/ O_RDWR
value.
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.
Okay, then please implement that and add a comment explaining why.
Just a heads up, this PR will stall for a while due to exam prep, but I will be back on it after rust week. |
This PR supports
F_SETFL
andF_GETFL
flags forfcntl
. In this implementation,F_SETFL
can only setO_NONBLOCK
flag.The interaction between these
fcntl
operations and blocking fd is summarised here.Fixes #4119