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.

Sign up for GitHub

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

Jump to bottom

Future-proof the kevent ABI #1572

Merged
merged 2 commits into from
May 8, 2022
Merged

Future-proof the kevent ABI #1572

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a24700b
Future-proof the kevent ABI
asomers May 7, 2022
8f0b8d5
If we're going to use mem::zeroed, we might as well use it.
asomers May 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 8 additions & 20 deletions src/sys/unix/selector/kqueue.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use crate::{Interest, Token};
use log::error;
use std::mem::MaybeUninit;
use std::mem::{self, MaybeUninit};
use std::ops::{Deref, DerefMut};
use std::os::unix::io::{AsRawFd, RawFd};
#[cfg(debug_assertions)]
Expand Down Expand Up @@ -34,17 +34,6 @@ type Flags = u16;
#[cfg(target_os = "netbsd")]
type Flags = u32;

// Type of the `data` field in the `kevent` structure.
#[cfg(any(
target_os = "dragonfly",
target_os = "freebsd",
target_os = "ios",
target_os = "macos"
))]
type Data = libc::intptr_t;
Copy link
Collaborator

Choose a reason for hiding this comment

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

DragonFlyBSD and MacOS (and likely iOS) still use intptr_t in the manual. I think to be safe we should stick to that as well. You can just move FreeBSD down to NetBSD and OpenBSD.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Except that FreeBSD 11 also still uses intptr_t. And currently there's no way for a crate to select which libc ABI it wants to use. So by removing that type entirely we'll just rely on type inference.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But you're using as i64 in multiple places below, so it doesn't use type interference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. I changed the argument type of kevent_register to use i64 everywhere, instead of being platform-dependent. So now there's only one place that needs type inference: line 284 (and line 61, before my second commit). Those are the only places that actually use the libc type.

#[cfg(any(target_os = "netbsd", target_os = "openbsd"))]
type Data = i64;

// Type of the `udata` field in the `kevent` structure.
#[cfg(not(target_os = "netbsd"))]
type UData = *mut libc::c_void;
Expand All @@ -57,9 +46,8 @@ macro_rules! kevent {
ident: $id as libc::uintptr_t,
filter: $filter as Filter,
flags: $flags,
fflags: 0,
data: 0,
udata: $data as UData,
..unsafe { mem::zeroed() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a note that this is needed for FreeBSD?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. Except, looking at it again, I realize that I should've just deleted the fflags and data field initializers.

}
};
}
Expand Down Expand Up @@ -163,7 +151,7 @@ impl Selector {
// the array.
slice::from_raw_parts_mut(changes[0].as_mut_ptr(), n_changes)
};
kevent_register(self.kq, changes, &[libc::EPIPE as Data])
kevent_register(self.kq, changes, &[libc::EPIPE as i64])
}

pub fn reregister(&self, fd: RawFd, token: Token, interests: Interest) -> io::Result<()> {
Expand Down Expand Up @@ -195,7 +183,7 @@ impl Selector {
kevent_register(
self.kq,
&mut changes,
&[libc::ENOENT as Data, libc::EPIPE as Data],
&[libc::ENOENT as i64, libc::EPIPE as i64],
)
}

Expand All @@ -211,7 +199,7 @@ impl Selector {
// the ENOENT error when it comes up. The ENOENT error informs us that
// the filter wasn't there in first place, but we don't really care
// about that since our goal is to remove it.
kevent_register(self.kq, &mut changes, &[libc::ENOENT as Data])
kevent_register(self.kq, &mut changes, &[libc::ENOENT as i64])
}

#[cfg(debug_assertions)]
Expand Down Expand Up @@ -264,7 +252,7 @@ impl Selector {
fn kevent_register(
kq: RawFd,
changes: &mut [libc::kevent],
ignored_errors: &[Data],
ignored_errors: &[i64],
) -> io::Result<()> {
syscall!(kevent(
kq,
Expand All @@ -289,11 +277,11 @@ fn kevent_register(
}

/// Check all events for possible errors, it returns the first error found.
fn check_errors(events: &[libc::kevent], ignored_errors: &[Data]) -> io::Result<()> {
fn check_errors(events: &[libc::kevent], ignored_errors: &[i64]) -> io::Result<()> {
for event in events {
// We can't use references to packed structures (in checking the ignored
// errors), so we need copy the data out before use.
let data = event.data;
let data = event.data as _;
// Check for the error flag, the actual error will be in the `data`
// field.
if (event.flags & libc::EV_ERROR != 0) && data != 0 && !ignored_errors.contains(&data) {
Expand Down