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

add graceful shim for the custom O_TMPFILE file opening flag plus test case #2718

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

Pointerbender
Copy link
Contributor

I'm trying if I can get the tempfile crate to work nicely with miri. Right now miri errors out due to an unsupported flag O_TMPFILE (= 0x410000) passed to OpenOptions::custom_flags. Interestingly, tempfile has a fallback in case the underlying file system does not support the O_TMPFILE flag, in which case open/open64 is expected to return the error code EOPNOTSUPP (= 95). This PR adds support for this scenario and also includes a test case (relevant zulip discussion).

src/shims/unix/fs.rs Outdated Show resolved Hide resolved
@Pointerbender
Copy link
Contributor Author

Thank you for the speedy review @RalfJung! I just pushed all the requested changes :)

src/shims/unix/fs.rs Outdated Show resolved Hide resolved
src/shims/unix/fs.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

RalfJung commented Dec 8, 2022

LGTM if CI passes, but please squash the commits.

@Pointerbender
Copy link
Contributor Author

Thanks! The CI passed just now and the commits are squashed, too 👍

@oli-obk
Copy link
Contributor

oli-obk commented Dec 8, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Dec 8, 2022

📌 Commit acb51b5 has been approved by oli-obk

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Dec 9, 2022

⌛ Testing commit acb51b5 with merge 2713dbd...

@bors
Copy link
Contributor

bors commented Dec 9, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 2713dbd to master...

@bors bors merged commit 2713dbd into rust-lang:master Dec 9, 2022
@@ -605,6 +605,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
// (Technically we do not support *not* setting this flag, but we ignore that.)
mirror |= o_cloexec;
}
if this.tcx.sess.target.os == "linux" {
let o_tmpfile = this.eval_libc_i32("O_TMPFILE")?;
if flag & o_tmpfile != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

As you noted, O_TMPFILE = 0x410000, which is more than one bit. This is for historical reasons, since the open() syscall ignores unrecognized flags: https://lwn.net/Articles/588444/

So the right check should be flag & o_tmpfile == o_tmpfile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Since this PR is already merged, I will address your feedback within my other related PR #2720

Copy link
Member

Choose a reason for hiding this comment

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

Since it looks like #2720 might be a bit more tricky, it'd be good to have a separate PR for this.

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 25, 2022
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Dec 28, 2022
spikespaz pushed a commit to spikespaz/dotwalk-rs that referenced this pull request Aug 29, 2024
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.

5 participants