implement better availability probing for copy_file_range #79274
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @nagisa |
| // To distinguish these cases we probe with invalid file descriptors which should result in EBADF if the syscall is supported | ||
| // and some other error (ENOSYS or EPERM) if it's not available | ||
| let available = match unsafe { | ||
| cvt(copy_file_range(-1, ptr::null_mut(), -1, ptr::null_mut(), 1, 0)) |
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`.
const INVALID_FD: _ = -1;| // and some other error (ENOSYS or EPERM) if it's not available | ||
| let available = match unsafe { | ||
| cvt(copy_file_range(-1, ptr::null_mut(), -1, ptr::null_mut(), 1, 0)) | ||
| } { |
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 { ... }
|
The review items should be addressed now. |
|
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.
|
@bors r+ |
|
📌 Commit 7647d03 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ 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.