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

signalfd optional file descriptor #1874

Merged
merged 1 commit into from
Dec 10, 2022

Conversation

JonathanWoollett-Light
Copy link
Contributor

@JonathanWoollett-Light JonathanWoollett-Light commented Nov 20, 2022

sys::signalfd::signalfd currently takes a RawFd for its fd argument.

Considering from the documentation:

If the fd argument is -1, then the call creates a new file
descriptor and associates the signal set specified in mask with
that file descriptor. If fd is not -1, then it must specify a
valid existing signalfd file descriptor, and mask is used to
replace the signal set associated with that file descriptor.

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.

EBADF The fd file descriptor is not a valid file descriptor.

This does however require additional changes to produce a cohesive implementation, notably changing the type within Signal from RawFd to ManuallyDrop<OwnedFd>, this has no functional affect, but illustrates ownership and allows the type to more easily produce BorrowedFds.

To use BorrowedFd requires updating the MSRV to >= 1.63.0

@asomers asomers added this to the 0.27.0 milestone Nov 20, 2022
@asomers asomers mentioned this pull request Nov 20, 2022
29 tasks
@SUPERCILEX
Copy link
Contributor

SUPERCILEX commented Nov 20, 2022

This should use impl AsFd instead of a BorrowedFd.

@@ -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> {
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member

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.

@JonathanWoollett-Light
Copy link
Contributor Author

JonathanWoollett-Light commented Nov 20, 2022

This should use impl AsFd instead of a BorrowedFd.

In my view this defats the purpose of this PR, which is to restrict the type you can pass to signalfd to a valid file descriptor such that the error case

EBADF The fd file descriptor is not a valid file descriptor.

Cannot be encountered.

With impl AsFd a user can still pass an invalid file descriptor.

@SUPERCILEX
Copy link
Contributor

With impl AsFd a user can still pass an invalid file descriptor.

Do you have a specific example in mind? Also, nothing prevents you from using unsafe to create a BorrowedFd from random garbage.

@JonathanWoollett-Light
Copy link
Contributor Author

Apologies I misread, thought you wrote AsRawFd. I will change it to impl AsFd.

@SUPERCILEX
Copy link
Contributor

Dope 👍

@JonathanWoollett-Light
Copy link
Contributor Author

JonathanWoollett-Light commented Nov 21, 2022

It does make specifying None a bit more of a pain:

signalfd(None::<OwnedFd>, mask, flags)?;

instead of

signalfd(None, mask, flags)?;

but this is relatively minor.

@SteveLauC
Copy link
Member

SteveLauC commented Nov 21, 2022

For the type of that file descriptor argument, any reason why not to use <F: AsFd>(fd: F), this is basically equivalent to impl AsFd except you can specify the particular type using turbofish, and this is consistent with the interfaces exposed from the stdlib:

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))
}

@JonathanWoollett-Light
Copy link
Contributor Author

JonathanWoollett-Light commented Nov 23, 2022

@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.

@asomers
Copy link
Member

asomers commented Dec 4, 2022

You should be able to rebase this now

@JonathanWoollett-Light
Copy link
Contributor Author

JonathanWoollett-Light commented Dec 4, 2022

You should be able to rebase this now

Nice, just rebased.

src/sys/signalfd.rs Outdated Show resolved Hide resolved
src/unistd.rs Outdated Show resolved Hide resolved
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

@bors bors bot merged commit df5877c into nix-rust:master Dec 10, 2022
@JonathanWoollett-Light JonathanWoollett-Light deleted the signalfd-option branch December 31, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants