-
Notifications
You must be signed in to change notification settings - Fork 681
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
signalfd optional file descriptor #1874
Conversation
93b29b7
to
d0ea228
Compare
This should use |
src/sys/signalfd.rs
Outdated
@@ -46,13 +46,14 @@ pub const SIGNALFD_SIGINFO_SIZE: usize = mem::size_of::<siginfo>(); | |||
/// signalfd (the default handler will be invoked instead). | |||
/// | |||
/// 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
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::signalfd
currently takes aRawFd
for itsfd
argument.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
Signal
fromRawFd
toManuallyDrop<OwnedFd>
, this has no functional affect, but illustrates ownership and allows the type to more easily produceBorrowedFd
s.To use
BorrowedFd
requires updating the MSRV to>= 1.63.0