add statx operation
#41
Conversation
| let leaked = Box::leak(inner); | ||
| *self = Self::Submitting(leaked as *mut _); |
There was a problem hiding this comment.
Box::into_raw exists as well, would give you the mut directly. Had not considered Box::leak could be used for this. I wonder if we could run miri on this codebase.
There was a problem hiding this comment.
| let leaked = Box::leak(inner); | |
| *self = Self::Submitting(leaked as *mut _); | |
| *self = Self::Submitting(Box::into_raw(inner)); |
However, the &mut existence at the same time is probably not an issue.
There was a problem hiding this comment.
miri
miri cannot talk to the OS sadly, as it runs in a VM. There is cargo-careful but it only does a fraction of what miri does (just enables some assertions in std instead of executing all code in a VM).
There was a problem hiding this comment.
However, the &mut existence at the same time is probably not an issue.
👍 .&mut exists at the same time as *mut does. There is no problem with that, the problem is only with &mut T vs &T and &mut T vs &mut T. The &mut T goes out of scope later on, so from what I can tell it's safe. It's definitely not beautiful though, Box::into_raw is better.
| let buf: Box<uring_common::libc::statx> = Box::new( | ||
| // TODO replace with Box<MaybeUninit>, https://github.com/rust-lang/rust/issues/63291 | ||
| // SAFETY: we only use the memory if the fstat succeeds, should be using MaybeUninit here. | ||
| unsafe { std::mem::zeroed() }, |
There was a problem hiding this comment.
Note the API which is unstable is Box::uninit, this is stable:
| let buf: Box<uring_common::libc::statx> = Box::new( | |
| // TODO replace with Box<MaybeUninit>, https://github.com/rust-lang/rust/issues/63291 | |
| // SAFETY: we only use the memory if the fstat succeeds, should be using MaybeUninit here. | |
| unsafe { std::mem::zeroed() }, | |
| let buf: Box<MaybeUninit<uring_common::libc::statx>> = Box::new( | |
| MaybeUninit::uninit() |
But without Box::assume_init this would require extra wrestling.
There was a problem hiding this comment.
extra wrestling
= keeping track of the MaybeUninit vs T state in the SubmittingBox.
| unsafe { | ||
| // It seems weird that current rust 1.75 Box::assume_init doesn't do the assert_inhabited | ||
| // that the regular MaybeUninit::assume_init does. Out of precaution, do that here. | ||
| statxbuf.assume_init_ref(); |
There was a problem hiding this comment.
I'll approve the PR right away, but this assume_init_ref call here was my idea. It might be that the compiler is now inserting these automatically whenever * (const|mut) MaybeInit<T> is cast to * (const|mut) T but as noted, the Box::assume_init right now does only have the cast.
I do think this cannot hurt, and this assuming init is what we are actually aiming to do here.
There was a problem hiding this comment.
Meant to tag @arpad-m in the above 🤦.
No description provided.