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

Port to rustix, and add support for using Linux's renameat2 #166

Merged
merged 4 commits into from
Jul 29, 2022

Conversation

sunfishcode
Copy link
Contributor

This ports tempfile to use the rustix crate instead of making direct libc calls, which factors out an unsafe block, simplifies error handling, and simplifies string handling.

While here, it also implements a suggestion in a comment; on recent Linux it uses renameat2 with RENAME_NOREPLACE to implement persist in non-overwrite mode.

This does require incrementing the minimum required Rust version to 1.48. Please feel free to decline this PR if you don't wish to take on these new requirements.

src/file/imp/unix.rs Outdated Show resolved Hide resolved
@polarathene
Copy link
Contributor

@Stebalien any concerns delaying review on this PR?


On a related note (due to the PR author), next month Rust 1.63 should have the new I/O Safety traits in std.

Until it's acceptable to raise the MSRV to >=1.63 to use those traits directly from std, in the meantime io-lifetimes could be adopted to cover these sections AFAIK?:

tempfile/src/file/mod.rs

Lines 968 to 988 in 716e58e

#[cfg(unix)]
impl<F> std::os::unix::io::AsRawFd for NamedTempFile<F>
where
F: std::os::unix::io::AsRawFd,
{
#[inline]
fn as_raw_fd(&self) -> std::os::unix::io::RawFd {
self.as_file().as_raw_fd()
}
}
#[cfg(windows)]
impl<F> std::os::windows::io::AsRawHandle for NamedTempFile<F>
where
F: std::os::windows::io::AsRawHandle,
{
#[inline]
fn as_raw_handle(&self) -> std::os::windows::io::RawHandle {
self.as_file().as_raw_handle()
}
}

pub fn reopen(file: &File, _path: &Path) -> io::Result<File> {
let handle = file.as_raw_handle();
unsafe {
let handle = ReOpenFile(
handle as HANDLE,
FILE_GENERIC_READ | FILE_GENERIC_WRITE,
FILE_SHARE_DELETE | FILE_SHARE_READ | FILE_SHARE_WRITE,
0,
);
if handle == INVALID_HANDLE_VALUE {
Err(io::Error::last_os_error())
} else {
Ok(FromRawHandle::from_raw_handle(handle as RawHandle))
}
}
}

@sunfishcode may offer to assist with that as they've expressed interest in contributing this improvement to popular crates, but it probably helps if their contribution efforts aren't left to stagnate without feedback? 😅

Copy link
Owner

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

Sorry, this fell off my radar. LGTM pending CI (which I'm trying to get to run now...).

@Stebalien
Copy link
Owner

Until it's acceptable to raise the MSRV to >=1.63 to use those traits directly from std, in the meantime sunfishcode/io-lifetimes#38 to cover these sections AFAIK?:

This seems reasonable to me. Just make sure to ping me when it's ready for review.

sunfishcode and others added 3 commits July 29, 2022 08:13
Port tempfile from libc to rustix. This simplifies error handling,
string conversion, and factors out an `unsafe` block.

This updates the MSRV to 1.48, which is rustix's MSRV.
Use `rustix::fs::renameat_with`, which corresponds to Linux's
`renameat2`, to implement the non-overlapping form of `persist`, on
Linux versions and filesystems where it's supported.
Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
@Stebalien Stebalien merged commit 5cffd50 into Stebalien:master Jul 29, 2022
Stebalien added a commit that referenced this pull request Jul 29, 2022
Unfortunately, this uses "raw" syscalls by default, which is too much of
a breaking change for users that expect to be able to override libc.

This reverts commit 5cffd50, reversing
changes made to 03c459a.
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.

3 participants