Fix kqueue filter consts type on NetBSD [WAS: Provide EV_SET] #787

vojtechkral · 2017-10-01T09:11:46Z

Using BSD kqueue's kevent structure is tedious, because some BSD variants define the structure slightly differently. This forces user's code to differentiate between BSD variants and initialize kevent accordingly, which is annoying and error-prone.

For an example, refer to MIO.

This is an attempt to fix it - provide a ctor function with the same signature across BSD variants.

Is an impl piece for a C structure allowed in libc?

edit: I noticed the kevent function has a similar problem, maybe I should include a fix for that too...
edit: ^ Done

alexcrichton · 2017-10-04T09:00:39Z

Thanks for the PR! Right now though this crate does not provide functions like this, this'd be an implementation detail in a downstream crate.

vojtechkral · 2017-10-04T09:29:44Z

Ok, understandable. Are macros out of question too? (Ie. maybe provide an EV_SET equivalent?) If so, I'll just deal with it in my own code, feel free to close this PR if this isn't applicable.

alexcrichton · 2017-10-04T21:51:06Z

Nah macros should be fine to add, there's already a number here

vojtechkral · 2017-10-05T09:31:57Z

Allrighty then, I'll refactor the code...

alexcrichton · 2017-10-05T20:39:07Z

Oh so actually you may want to look for invocations of the f! macro, we typically provide functions for C macros rather than literal Rust macros

vojtechkral · 2017-10-08T13:29:51Z

Ok, take three using f!. I ended up figuring out defining a couple of helper type aliases is probably the best way. I also noticed kevent filter consts have a wrong type on netbsd.

alexcrichton · 2017-10-09T10:59:07Z

src/unix/bsd/apple/mod.rs

+pub type kevent_nevents = ::c_int;
+pub type kevent_filter = ::int16_t;
+pub type kevent_flags = ::uint16_t;
+pub type kevent_fflags = ::uint32_t;


Hm if these don't actually exist, how come we're defining them?

Well, the idea is that, for example, kevent_nevents is "the type that the kevent function uses for the nevents param on this BSD variant" (it varies). So that in user code I can cast a variable as kevent_nevents without having to worry about which particular BSD I'm currently on. Same for the other kevent_* types.

That makes sense, yes, but it's not what libc does. We don't add things that aren't in headers.

alexcrichton · 2017-10-09T10:59:56Z

src/unix/bsd/apple/mod.rs

        _WSTATUS(status) == _WSTOPPED && WSTOPSIG(status) != 0x13
    }
+
+    pub fn EV_SET(ident: ::uintptr_t, filter: kevent_filter, flags: kevent_flags,


These macros aren't like their C counterparts where they take a kevent as the first argument?

Oh, yes, I wrote it this way so that you don't need to first create an uninitialized kevent struct. But I don't have a strong opinion on this...

I think we should stick to the C macros as much as possible

vojtechkral · 2017-10-09T12:54:39Z

I'll fix those long lines...

vojtechkral · 2017-10-13T19:14:00Z

Ok, so, since platform abstractions aren't allowed, I guess I'll just make this into the netbsd const fixes.

The only other way of making EV_SET independent of the underlying integer type would be to make it generic with Into<someCommonInteger>, but that's not supported by the f! macro and I don't know whether it's allowed in terms of policy (I presume not). So, there's no real reason to include EV_SET since it wouldn't really do anything...

alexcrichton · 2017-10-14T14:50:53Z

Ok, sounds like EV_SET should be left out?

vojtechkral · 2017-10-23T22:16:23Z

Sorry for the delay. So I just reduced this to the fix for NetBSD. (Waiting for CI now.)

alexcrichton · 2017-10-24T13:26:56Z

@bors: r+

bors · 2017-10-24T13:33:10Z

📌 Commit a0e683e has been approved by alexcrichton

bors · 2017-10-24T13:35:15Z

⌛ Testing commit a0e683e with merge 42b24bf...

Fix kqueue filter consts type on NetBSD [WAS: Provide EV_SET] Using BSD kqueue's `kevent` structure is tedious, because some BSD variants define the structure slightly differently. This forces user's code to differentiate between BSD variants and initialize `kevent` accordingly, which is annoying and error-prone. For an example, refer to [MIO](https://github.com/carllerche/mio/blob/master/src/sys/unix/kqueue.rs#L38). This is an attempt to fix it - provide a ctor function with the same signature across BSD variants. Is an `impl` piece for a C structure allowed in libc? **edit:** I noticed the `kevent` function has a similar problem, maybe I should include a fix for that too... **edit:** ^ Done

bors · 2017-10-24T13:58:59Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 42b24bf to master...

vojtechkral force-pushed the master branch 2 times, most recently from 930b50d to f7c3e97 Compare October 1, 2017 13:51

vojtechkral force-pushed the master branch from f7c3e97 to 5daaeb3 Compare October 5, 2017 17:26

vojtechkral changed the title ~~Provide kevent::new(), an EV_SET analogue~~ Provide EV_SET Oct 5, 2017

vojtechkral force-pushed the master branch from 5daaeb3 to db1b27d Compare October 8, 2017 13:27

vojtechkral force-pushed the master branch from db1b27d to 33bb911 Compare October 8, 2017 13:38

alexcrichton reviewed Oct 9, 2017

View reviewed changes

Fix kqueue filter consts type on NetBSD

a0e683e

vojtechkral force-pushed the master branch from 33bb911 to a0e683e Compare October 23, 2017 22:15

vojtechkral changed the title ~~Provide EV_SET~~ Fix kqueue filter consts type on NetBSD [WAS: Provide EV_SET] Oct 23, 2017

bors merged commit a0e683e into rust-lang:master Oct 24, 2017

lucab mentioned this pull request Oct 26, 2017

sys/socket: add UnixAddr abstract name getter nix-rust/nix#785

Merged

TethysSvensson mentioned this pull request Mar 3, 2019

Fix the sign of the EVFILT_* defines #506

Closed

Fix kqueue filter consts type on NetBSD [WAS: Provide EV_SET] #787

Fix kqueue filter consts type on NetBSD [WAS: Provide EV_SET] #787

Uh oh!

Conversation

vojtechkral commented Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented Oct 4, 2017

Uh oh!

vojtechkral commented Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented Oct 4, 2017

Uh oh!

vojtechkral commented Oct 5, 2017

Uh oh!

alexcrichton commented Oct 5, 2017

Uh oh!

vojtechkral commented Oct 8, 2017

Uh oh!

alexcrichton Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

vojtechkral Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

alexcrichton Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

alexcrichton Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

vojtechkral Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

alexcrichton Oct 9, 2017

Choose a reason for hiding this comment

Uh oh!

vojtechkral commented Oct 9, 2017

Uh oh!

vojtechkral commented Oct 13, 2017

Uh oh!

alexcrichton commented Oct 14, 2017

Uh oh!

vojtechkral commented Oct 23, 2017

Uh oh!

alexcrichton commented Oct 24, 2017

Uh oh!

bors commented Oct 24, 2017

Uh oh!

bors commented Oct 24, 2017

Uh oh!

bors commented Oct 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vojtechkral commented Oct 1, 2017 •

edited

Loading

vojtechkral commented Oct 4, 2017 •

edited

Loading