Take OwnedFd instead of RawFd in new_from_fd
#45
Conversation
This is a breaking API change and requires Rust 1.63. The function still needs to be `unsafe` since the fd is mmaped, but this is more idiomatic and makes it clear that the file descriptor is owned rather than borrowed. (Which should match the existing implementation). Though maybe it should only borrow the FD?
|
Looking at the implementation, it seems that |
In fact, no. Your PR is correct. |
| size: usize, | ||
| format: KeymapFormat, | ||
| flags: KeymapCompileFlags, | ||
| ) -> std::io::Result<Option<Keymap>> { | ||
| let map = MmapOptions::new() | ||
| .len(size as usize) | ||
| // Starting in version 7 of the wl_keyboard protocol, the keymap must be mapped using MAP_PRIVATE. | ||
| .map_copy_read_only(&fs::File::from_raw_fd(fd))?; | ||
| .map_copy_read_only(&fs::File::from(fd))?; |
There was a problem hiding this comment.
Why convert to a file? &fd works just fine.
There was a problem hiding this comment.
I just preserved the behavior it had before, with a type that communicates it.
It looks like map_copy_read_only in memmap2 used to take an &File, so that's probably why a conversion was used here. But now uses a trait MmapAsRawDesc, which works of RawFd and references to types implementing AsRawFd.
So this change works. It also should be possible to accepted a BorrowedFd. (The caller needs to ensure the data isn't mutated while mapped for safety regardless.)
This is a breaking API change and requires Rust 1.63.
The function still needs to be
unsafesince the fd is mmaped, but this is more idiomatic and makes it clear that the file descriptor is owned rather than borrowed. (Which should match the existing implementation). Though maybe it should only borrow the FD?It wasn't documented explicitly if this took ownership of the fd, and this wasn't handled correctly in smithay-client-toolkit: Smithay/client-toolkit#412.