-
Notifications
You must be signed in to change notification settings - Fork 724
signalfd optional file descriptor #1874
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
Conversation
93b29b7 to
d0ea228
Compare
|
This should use |
src/sys/signalfd.rs
Outdated
| /// | ||
| /// See [the signalfd man page for more information](https://man7.org/linux/man-pages/man2/signalfd.2.html) | ||
| pub fn signalfd(fd: RawFd, mask: &SigSet, flags: SfdFlags) -> Result<RawFd> { | ||
| pub fn signalfd(fd: Option<BorrowedFd>, mask: &SigSet, flags: SfdFlags) -> Result<OwnedFd> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way we could use the io-lifetimes crate here to avoid bumping the MSRV?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes since that's what rustix does, but not sure that's worth it without someone piping up to give a compelling reason they need to be able to use old rustcs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already planning to raise MSRV for the 0.27.0 release.
In my view this defats the purpose of this PR, which is to restrict the type you can pass to
Cannot be encountered. With |
983454e to
7fa6f90
Compare
Do you have a specific example in mind? Also, nothing prevents you from using unsafe to create a BorrowedFd from random garbage. |
|
Apologies I misread, thought you wrote |
|
Dope 👍 |
|
It does make specifying signalfd(None::<OwnedFd>, mask, flags)?;instead of signalfd(None, mask, flags)?;but this is relatively minor. |
6c4312e to
b2a3ed7
Compare
|
For the type of that file descriptor argument, any reason why not to use pub fn fchown<F: AsFd>(fd: F, uid: Option<u32>, gid: Option<u32>) -> io::Result<()> {
sys::fs::fchown(fd.as_fd().as_raw_fd(), uid.unwrap_or(u32::MAX), gid.unwrap_or(u32::MAX))
} |
d9a45a4 to
99e8173
Compare
|
@SteveLauC I have no strong view either way as you mention they are essentially equivalent, I've changed it to match stdlib as you mentioned. |
|
You should be able to rebase this now |
99e8173 to
c49139a
Compare
Nice, just rebased. |
c49139a to
08edc80
Compare
08edc80 to
82e46ab
Compare
82e46ab to
147ed3f
Compare
asomers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors r+
sys::signalfd::signalfdcurrently takes aRawFdfor itsfdargument.Considering from the documentation:
We can better pass the argument as
Option<BorrowedFd>which encodes the optional nature of this parameter in an option rather than the value being -1 (invalid) (size_of::<Option<BorrowedFd>>() == size_of::<RawFd>() == 4).This removes the error case where
fd < -1.This does however require additional changes to produce a cohesive implementation, notably changing the type within
SignalfromRawFdtoManuallyDrop<OwnedFd>, this has no functional affect, but illustrates ownership and allows the type to more easily produceBorrowedFds.To use
BorrowedFdrequires updating the MSRV to>= 1.63.0