search

rust-x-bindings / xkbcommon-rs

bindings and safe wrappers for libxkbcommon
MIT License
20 stars 24 forks source link

Take `OwnedFd` instead of `RawFd` in `new_from_fd` #45

Closed ids1024 closed 1 year ago

ids1024 commented 1 year ago

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?

It wasn't documented explicitly if this took ownership of the fd, and this wasn't handled correctly in smithay-client-toolkit: https://github.com/Smithay/client-toolkit/pull/412.

rtbo commented 1 year ago

Looking at the implementation, it seems that RawFd is the correct type here. The fd is used to map the file to memory, and the map is dropped by when the function returns. So the Keymap doesn't keep any reference to the file whatsoever.

rtbo commented 1 year ago

it seems that RawFd is the correct type here

In fact, no. Your PR is correct. File::from_raw_fd consumes the descriptor, and closes it just after the memory is mapped.