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

feat: Expose other kqueue filters #83

Merged
merged 9 commits into from
Feb 3, 2023
Merged

Conversation

notgull
Copy link
Member

@notgull notgull commented Feb 1, 2023

This PR uses extension traits to expose other kqueue filters, like signals and child processes.

Supercedes #72

Closes #68

@notgull notgull marked this pull request as ready for review February 1, 2023 17:55
@notgull notgull requested a review from fogti February 2, 2023 14:39
src/os.rs Outdated Show resolved Hide resolved
src/os.rs Outdated Show resolved Hide resolved
Copy link
Member

@fogti fogti left a comment

Choose a reason for hiding this comment

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

considerations regarding #[inline] and pub

src/os/kqueue.rs Show resolved Hide resolved
src/os/kqueue.rs Show resolved Hide resolved
src/os/kqueue.rs Outdated
/// No matter what `PollMode` is specified, this filter will always be
/// oneshot-only.
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct Signal(c_int);
Copy link
Member

Choose a reason for hiding this comment

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

it would make more sense to just make the single wrapped field pub and omit the construct/destruct methods.

src/os/kqueue.rs Outdated
Comment on lines 223 to 227
/// Identifier for the timer.
id: usize,

/// The timeout to wait for.
timeout: Duration,
Copy link
Member

Choose a reason for hiding this comment

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

consider marking these pub.

src/os/kqueue.rs Outdated

impl Timer {
/// Create a new timer.
pub fn new(id: usize, timeout: Duration) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

if the fields of this struct would be marked pub, these methods could be omitted.

src/os/kqueue.rs Outdated Show resolved Hide resolved
@fogti
Copy link
Member

fogti commented Feb 3, 2023

suggestion: also uhh it's suboptimal that you put 2 independent changes into the last commit (one which moved the file, and a fixup), that makes it harder to review, would be nice if you could avoid that in the future...

looks otherwise pretty ok now. thx

@notgull
Copy link
Member Author

notgull commented Feb 3, 2023

Sorry about that, and thanks for the review!

Regarding the pub changes: I originally wanted to do this, but then I realize that would force us into a corner if new features are added to the kqueue API. For instance, if a new fflags option is added to a filter (this has happened relatively recently in NetBSD), we wouldn't be able to express it, since adding a new field to that structure would be a breaking change. Having the structs have private fields future-proofs us for future changes to kqueue.

@fogti
Copy link
Member

fogti commented Feb 3, 2023

oh btw the reason I suggested pub is that if new features would be added, it would make sense that the API would need to adjusted anyways (although a builder-like pattern would be possible, it might make the current interface awkward).

@notgull
Copy link
Member Author

notgull commented Feb 3, 2023

if new features would be added, it would make sense that the API would need to adjusted anyways

Good point. I've updated it so the fields are pub, except for the Process filter, since that one is slightly more complex than the others.

@notgull notgull merged commit a5aae98 into master Feb 3, 2023
@notgull notgull deleted the notgull/kqueue-other-filters branch February 3, 2023 19:14
notgull added a commit that referenced this pull request Feb 21, 2023
* feat: Expose other kqueue filters

* Fix netbsd/openbsd compilation

* Build MSRV for FreeBsd/OpenBsd in CI

* Only run MSRV BSD builds on Linux

* Change API a little + fix netbsd timer

* Add inlines + move PollerSealed

* rustfmt

* Make filter fields public

* Fix examples
@notgull notgull mentioned this pull request Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add support for custom kqueue filters
2 participants