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

Make Flock implement AsRawFd #2571

Closed
redactedontop opened this issue Dec 25, 2024 · 9 comments
Closed

Make Flock implement AsRawFd #2571

redactedontop opened this issue Dec 25, 2024 · 9 comments

Comments

@redactedontop
Copy link

redactedontop commented Dec 25, 2024

Hello. I'm currently trying to put a flock inside a mmap, and as it doesn't implement AsRawFd, memmap2 doesn't allow it. I have to do these shenanigans:

let mut file = unsafe { File::from_raw_fd(Flock::lock(file, FlockArg::LockExclusive).ok()?.as_raw_fd()) }; // SAFETY: Raw FD is copied from an existing file

Could AsRawFd be implemented into Flock?
Thank you,
Alex <3

@SteveLauC
Copy link
Member

Could you please show me more code? Specifically, the code that requires you to do these shenanigans?

And, yeah, we can impl AsRawFd for Flock using flock.deref().as_raw_fd()

@redactedontop
Copy link
Author

2 code samples:

  1. let mut file = unsafe { File::from_raw_fd(Flock::lock(file, FlockArg::LockExclusive).ok()?.as_raw_fd()) }; // SAFETY: Raw FD is copied from an existing file let mut mmap = unsafe { mmap.map_mut(&file).ok()? }; // SAFETY: file is auto-locked on Windows and manually locked on Linux

  2. let mut file = unsafe { File::from_raw_fd(Flock::lock(file, FlockArg::LockExclusive).ok()?.as_raw_fd()) }; // SAFETY: Raw FD is copied from an existing file file.seek(SeekFrom::End(-32)).ok()?; let mut mmap = unsafe { mmap.map_mut(&file).ok()? }; // SAFETY: file is auto-locked on Windows and manually locked on Linux

@redactedontop
Copy link
Author

Only &file implements AsRawHandle on Windows and if I deref file I get a &&file which doesn't impl AsRawFd.

@SteveLauC
Copy link
Member

use std::{fs::File, os::fd::AsRawFd};
use nix::fcntl::{Flock, FlockArg};

fn foo<FD: AsRawFd>(_: FD) {}

fn main() {
    let file = File::open("Cargo.toml").unwrap();
    let lock_guard = Flock::lock(file, FlockArg::LockExclusive).unwrap();
    foo(lock_guard); // Error: the trait bound `Flock<File>: AsRawFd` is not satisfied
}

Thanks for providing more code, I guess you are basically doing the above thing.

Flock is a lock guard, and AsRawFd should only be implemented for file descriptors, so IMHO we should probably not implement it for this semantic reason.

Could you do conditional compilation here?

@SteveLauC
Copy link
Member

But there is indeed one thing that makes me think we can implement it, the only trait bound of Flockable is AsRawFd, so implementing AsRawFd for Flock can be seen as a shortcut for flock.deref().as_raw_fd() (if we ignore Rust's recursive method llookup

@redactedontop
Copy link
Author

You can, I already checked. I can PR if needed.

@SteveLauC
Copy link
Member

Gentle ping on @asomers, thoughts on this? Is implementing AsRawFd for Flock semantically right in your opinion?

@asomers
Copy link
Member

asomers commented Jan 3, 2025

The fact that we can already do flock.deref().as_raw_fd() means there's no additional safety issue with implementing AsRawFd directly. The only problems I foresee are those that arise everywhere AsRawFd is used. That's a big problem, because RawFd is Clone, which makes it easy to violate Flock's exclusive access guarantee. But that was already possible, thanks to Deref. I think this change should be allowed, until and unless std makes RawFd inherently unsafe.

@SteveLauC
Copy link
Member

Makes sense, thanks for being here.

I am gonna close this issue because this feature request

  1. does not semantically look right in my opinion
  2. Will make the safety guarantee worse, as Alan said

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants