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

Rustier kqueue API #1943

Merged
merged 1 commit into from
Feb 10, 2023
Merged

Rustier kqueue API #1943

merged 1 commit into from
Feb 10, 2023

Conversation

asomers
Copy link
Member

@asomers asomers commented Dec 13, 2022

  • Prefer methods instead of functions.
  • Create a newtype for a kqueue.
  • Document everything.
  • Deprecate EVFILT_SENDFILE, because it was never fully implemented upstream.
  • Add support to the libc_enum! macro to be able to deprecate variants.

EVFILT_LIO,
#[cfg(any(target_os = "ios", target_os = "macos"))]
/// Mach portsets
EVFILT_MACHPORT,
Copy link
Contributor

@notgull notgull Dec 13, 2022

Choose a reason for hiding this comment

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

This part may be unsound; I'm not sure about mach port support here but it might not be just an FD.

Copy link
Member Author

Choose a reason for hiding this comment

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

What might not just be an FD? And what part might be unsound? This just defines the filter constant.

impl Kqueue {
/// Create a new kernel event queue.
pub fn new() -> Result<Self> {
let res = unsafe { libc::kqueue() };
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 plan to support the kqueue1() function exposed by FreeBSD?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean NetBSD? No. I don't know of anybody who needs it, and we don't have a dedicated NetBSD maintainer.

src/sys/event.rs Outdated
use std::ptr;

// Redefine kevent in terms of programmer-friendly enums and bitfields.
/// A Kernel event queue. Used to notify a process of various asynchronous
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit.

Suggested change
/// A Kernel event queue. Used to notify a process of various asynchronous
/// A kernel event queue. Used to notify a process of various asynchronous

}

/// Modify an existing [`KEvent`].
// Probably should deprecate. Would anybody ever use it over `KEvent::new`?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think deprecating it makes sense. If there is a use case I expect we'll hear about it after marking it as deprecated - we can always remove the deprecation label and keep it around if a use case does come up.

NOTE_VM_PRESSURE_TERMINATE;
#[allow(missing_docs)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we find ourselves sprinkling #[allow(missing_docs)] over every attribute like this in other structs, we might consider modifying the libc_bitflags macro to make it less tedious.

Copy link
Collaborator

@rtzoeller rtzoeller left a comment

Choose a reason for hiding this comment

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

Approved with some minor feedback. Don't forget to update the CHANGELOG PR #.

* Prefer methods instead of functions.
* Create a newtype for a kqueue.
* Document everything.
* Deprecate EVFILT_SENDFILE, because it was never fully implemented
  upstream.
* Add support to the libc_enum! macro to be able to deprecate variants.
@asomers
Copy link
Member Author

asomers commented Feb 10, 2023

bors r=rtzoeller

@bors bors bot merged commit 2a40128 into nix-rust:master Feb 10, 2023
@SteveLauC SteveLauC mentioned this pull request Nov 12, 2023
29 tasks
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.

None yet

3 participants