Uh oh!
There was an error while loading. Please reload this page.
implement better availability probing for copy_file_range - #79274
Conversation
rust-highfive
commented
Nov 21, 2020
(rust_highfive has picked a reviewer for you, use r? to override) |
Dylan-DPC-zz
commented
Dec 8, 2020
r? @nagisa |
nagisa
left a comment
There was a problem hiding this comment.
Broadly LGTM. See inline comments for small nits. The constant magic number thing is the only blocker.
There was a problem hiding this comment.
Can you add a short blurb explaining why -1 is the descriptor we want to use here (and why it is guaranteed to be invalid)? Maybe even extract the magic number into constant? Maybe like this:
/// Invalid file descriptor.////// Valid file descriptors on UNIX are specified to be positive as per e.g. documentation of `open`.constINVALID_FD:_ = -1;There was a problem hiding this comment.
Style nit: This match can probably be more concisely written as such:
let available = matches!(cvt(...).map_err(|e| e.raw_os_error()),Err(Some(libc::EBADF)))If you decide you want to continue using match, I would suggest extracting the expression passed to match into a separate variable, because match COMPLEX_EXPRESSION { ... } tends to look pretty bad when auto-formatted.
let result = unsafe{cvt(...)};let available = match result { ...}the8472
commented
Dec 9, 2020
The review items should be addressed now. |
Uh oh!
There was an error while loading. Please reload this page.
the8472
commented
Dec 9, 2020
Yeah, didn't run tests before pushing. One moment. |
previously any attempt to copy to an immutable file (EPERM) would disable copy_file_range support for the whole process.
nagisa
commented
Dec 9, 2020
@bors r+ |
bors
commented
Dec 9, 2020
📌 Commit 7647d03 has been approved by |
bors
commented
Dec 10, 2020
bors
commented
Dec 10, 2020
☀️ Test successful - checks-actions |
Followup to #75428 (comment)
Previously syscall detection was overly pessimistic. Any attempt to copy to an immutable file (EPERM) would disable copy_file_range support for the whole process.
The change tries to copy_file_range on invalid file descriptors which will never run into the immutable file case and thus we can clearly distinguish syscall availability.