WIP: Change kevent function signature to use Timespec #838

Thomasdezeeuw · 2018-01-16T18:00:26Z

Updates #516.

Don't merge yet. This is just to agree an API first.

Removes kevent_ts.

asomers · 2018-01-17T01:20:25Z

I think this is just what we want for an API.

Susurrus · 2018-01-17T02:10:30Z

test/sys/test_event.rs

+    let timeout = timespec { tv_sec: 0, tv_nsec: 0};
+    assert_eq!(kevent(kq, &[], &mut events, Some(timeout)).unwrap(), 0);
+
+    assert_eq!(kevent::<timespec>(kq, &[], &mut events, None).unwrap(), 0);


This is the unfortunate side effect of these option types. I'd suggest having these all as doctests instead of integration tests, since these examples will be helpful for users.

Susurrus · 2018-01-17T02:10:33Z

src/sys/event.rs

-              changelist: &[KEvent],
-              eventlist: &mut [KEvent],
-              timeout_opt: Option<timespec>) -> Result<usize> {
+pub fn kevent<T: Into<TimeSpec>>(kq: RawFd, changelist: &[KEvent], eventlist: &mut [KEvent], timeout: Option<T>) -> Result<usize> {


Please add a doccomment since you're editing this function.

I've documented the entire module now.

Susurrus · 2018-01-17T02:11:30Z

src/sys/time.rs

@@ -58,6 +60,20 @@ const TS_MAX_SECONDS: i64 = ::std::isize::MAX as i64;

 const TS_MIN_SECONDS: i64 = -TS_MAX_SECONDS;

+impl From<Duration> for TimeSpec {


I think there should be some doctests for TimeSpec demonstrating these conversions as they can be hard to discover unless we put them in examples.

Susurrus · 2018-01-17T02:16:33Z

test/sys/test_event.rs

+    let mut events = Vec::new();
+
+    let timeout = Duration::from_millis(0);
+    assert_eq!(kevent(kq, &[], &mut events, Some(timeout)).unwrap(), 0);


With the above function call change you can get rid of the Some(...) wrapping.

Susurrus · 2018-01-17T02:17:45Z

src/sys/time.rs

+impl From<Duration> for TimeSpec {
+    fn from(duration: Duration) -> Self {
+        TimeSpec(timespec{
+            tv_sec: cmp::min(duration.as_secs(), time_t::max_value() as u64) as time_t,


I'm a little wary of this conversion. How do we document this and also how do we report an error to the user? I don't like just capping this value, I'd actually suggest a panic! instead.

I don't think the conversion really matter since it's silently capped to 24 hours by the OS in any case. But I see it panic on i386, so I'll have to fix that anyway.

That of course only applied to kevent, I'll fix this as well.

Susurrus · 2018-01-17T02:22:05Z

src/sys/event.rs

-              changelist: &[KEvent],
-              eventlist: &mut [KEvent],
-              timeout_opt: Option<timespec>) -> Result<usize> {
+pub fn kevent<T: Into<TimeSpec>>(kq: RawFd, changelist: &[KEvent], eventlist: &mut [KEvent], timeout: Option<T>) -> Result<usize> {


Instead pub fn kevent<T: Into<Option<TimeSpec>>>(kq: RawFd, changelist: &[KEvent], eventlist: &mut [KEvent], timeout: T) -> Result<usize>

I don't think that is possible, since you'll get trait bound issue such as: 'std::convert::From<std::time::Duration>' is not implemented for 'std::option::Option<nix::sys::time::TimeSpec>'

Including examples about converting `Duration` and `libc::timespec` into `TimeSpec`.

Changes the `ident` parameter to use `uintptr_t`, rather then `usize`.

This has moved into the kevent documentation.

And fixes a type-o in the TODO.

Thomasdezeeuw · 2018-01-17T11:21:27Z

I've expanded the PR a bit and documented the entire module.

The only thing left trying to get kevent<T: Into<Option<TimeSpec>>>() to work, but I couldn't.

Susurrus · 2018-01-17T15:21:33Z

You can get the Into<Option<..>> change working by just calling .into() the normal way:

diff --git a/src/sys/event.rs b/src/sys/event.rs
index 5db2aaf8..26df5373 100644
--- a/src/sys/event.rs
+++ b/src/sys/event.rs
@@ -334,8 +334,8 @@ type type_of_nchanges = size_t;
 /// let timeout = timespec { tv_sec: 0, tv_nsec: 1000};
 /// assert_eq!(kevent(kq, &[], &mut events, Some(timeout)).unwrap(), 0);
 /// ```
-pub fn kevent<T: Into<TimeSpec>>(kq: RawFd, changelist: &[KEvent], eventlist: &mut [KEvent], timeout: Option<T>) -> Result<usize> {
-    let timeout = timeout.map(|t| t.into());
+pub fn kevent<T: Into<Option<TimeSpec>>>(kq: RawFd, changelist: &[KEvent], eventlist: &mut [KEvent], timeout: T) -> Result<usize> {
+    let timeout = timeout.into();
     let timeout_ptr = if let Some(ref timeout) = timeout {
         timeout.as_ref() as *const timespec
     } else {

Built this locally with cargo build --target=x86_64-unknown-freebsd, so it should be right.

Susurrus · 2018-01-17T15:36:47Z

Also, great work on the docs. Would you be able to add docs for each of the enum variants as well? Then we can set #[deny(missing_docs)] before pub mod event in src/sys/mod.rs to enforce docs for this entire module.

I also wanted to ask if this API made sense. For example, I don't get why ev_set in the Rust API, when we can just have setters for each struct member, which seems more intuitive to me. If we document that you don't need to use ev_set and instead modify the struct using setters, I imagine users will understand just fine.

Thomasdezeeuw · 2018-01-18T12:05:45Z

For Rust ev_set doesn't really make sense. I think getters and setters, or all public fields, would be better.

Thomasdezeeuw · 2018-01-18T12:50:53Z

@Susurrus I still can't get the API to work, I only got it to run with two generic types: https://play.rust-lang.org/?gist=ccaf7dff4182bba7dfcc7d6958fa0b9c&version=stable.

c_long on 32 bit is `i32`, which doesn’t implement `From<u32>`.

This doesn’t really make sense as a Rust API.

Thomasdezeeuw · 2018-01-19T13:51:39Z

@Susurrus Are you sure want all the enum variants documented? There could be slight differences on each platform/OS and people should really consult the appropriate manual for that. I could copy from those manuals for each variant, but to be honst I don't the point in doing that.

Susurrus · 2018-01-19T17:21:02Z

I'm going to dig into the kevent() issue with trying to get ridof wrapping the argument in Some(), but I wanted to comment on the enum variant documentation: absolutely they should be documented. The solution to differences in the variants across platforms isn't less documentation it's more documentation. If you look through any of the last PRs it's something I've been hammering on the last 6 months and we've actually finally started to get clear docs. The goal of nix isn't just to make it easier for people who already know these interfaces to use them, but also to make them usable for people who never would have touched them before! We should enable those users, not leave it hard "because it should be hard". And we might run into issues with our documentation being wrong, but I expect that to be rare, as we haven't seen too much of it as of yet and nix is a pretty heavily used crate.

Thomasdezeeuw · 2018-01-21T18:10:05Z

@Susurrus I agree that nix makes system call easier and that good documentation is very important But I'm not sure that nix should be location for that documentation. Most functions refer to man pages on opengroup, which should really be read before using most system calls.

For kevent espcially; because it can add, remove and change receiving of notifcations for different operations such as reading and writing, and it can do this for file descriptors, processes, signals and can even control timers. It such a generic function with so many capabilities that it is hard to use. But is well documented and if you want you use the best way to learn about it is by reading the manual pages.

So again I agree; documentation is important. But I'm arguing that it's better to link to the documentation of the implementation (or specficication if any), rather then keeping that documentation in the nix crate. Is very likely that most of the documentation in the nix crate would overlap with the man pages, but in all likelyhood isn't as actively updated as the actual man pages.

Just my two cents.

Thomasdezeeuw · 2019-02-18T08:30:53Z

It's been a year, so I'm going to close this.

Thomasdezeeuw added 4 commits January 16, 2018 18:43

Implement From<timespec> for Timespec

72e0192

Implement From<Duration> for Timespec

f11d041

Change kevent to use Into<Timespec>

5705872

Removes kevent_ts.

Add a simple test for kevent

f7d60e9

Thomasdezeeuw changed the title ~~Duration~~ Change kevent function signature to use Timespec Jan 16, 2018

Thomasdezeeuw mentioned this pull request Jan 16, 2018

Use better types for all timeout arguments #516

Open

asomers changed the title ~~Change kevent function signature to use Timespec~~ WIP: Change kevent function signature to use Timespec Jan 17, 2018

Susurrus suggested changes Jan 17, 2018

View reviewed changes

Thomasdezeeuw added 15 commits January 17, 2018 11:34

Add documentation to TimeSpec

6564a0b

Including examples about converting `Duration` and `libc::timespec` into `TimeSpec`.

Change ev_set signature to match KEvent::new

f943049

Changes the `ident` parameter to use `uintptr_t`, rather then `usize`.

Document ev_set

ab366d2

Document kqueue

4b0e1ab

Document kevent

ab1363c

Remove test_kevent

b03d0a0

This has moved into the kevent documentation.

Document FilterFlag

f85707c

Document EventFlag

b214950

Document EventFilter

0895e01

Document KEvent

9204b8e

Document type_of_event_flag

4795e22

Document the event module

222a7dd

And fixes a type-o in the TODO.

Fix TimeSpec example

5b4f3f6

Fix conversion of Duration into TimeSpec

6f039df

Add changes regarding kevent to changelog

bf9cf68

Fix build errors on 32bit architectures

015231c

c_long on 32 bit is `i32`, which doesn’t implement `From<u32>`.

Thomasdezeeuw added 2 commits January 19, 2018 14:49

Remove ev_set

ea55514

This doesn’t really make sense as a Rust API.

Document all methods on KEvent

ecc9924

Thomasdezeeuw closed this Feb 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Change kevent function signature to use Timespec #838

WIP: Change kevent function signature to use Timespec #838

Thomasdezeeuw commented Jan 16, 2018

asomers commented Jan 17, 2018

Susurrus Jan 17, 2018

Susurrus Jan 17, 2018

Thomasdezeeuw Jan 17, 2018

Susurrus Jan 17, 2018

Thomasdezeeuw Jan 17, 2018

Susurrus Jan 17, 2018

Susurrus Jan 17, 2018

Thomasdezeeuw Jan 17, 2018

Thomasdezeeuw Jan 17, 2018

Susurrus Jan 17, 2018

Thomasdezeeuw Jan 17, 2018

Thomasdezeeuw commented Jan 17, 2018

Susurrus commented Jan 17, 2018

Susurrus commented Jan 17, 2018

Thomasdezeeuw commented Jan 18, 2018

Thomasdezeeuw commented Jan 18, 2018

Thomasdezeeuw commented Jan 19, 2018

Susurrus commented Jan 19, 2018

Thomasdezeeuw commented Jan 21, 2018

Thomasdezeeuw commented Feb 18, 2019

		@@ -58,6 +60,20 @@ const TS_MAX_SECONDS: i64 = ::std::isize::MAX as i64;

		const TS_MIN_SECONDS: i64 = -TS_MAX_SECONDS;

		impl From<Duration> for TimeSpec {

WIP: Change kevent function signature to use Timespec #838

WIP: Change kevent function signature to use Timespec #838

Conversation

Thomasdezeeuw commented Jan 16, 2018

asomers commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thomasdezeeuw commented Jan 17, 2018

Susurrus commented Jan 17, 2018

Susurrus commented Jan 17, 2018

Thomasdezeeuw commented Jan 18, 2018

Thomasdezeeuw commented Jan 18, 2018

Thomasdezeeuw commented Jan 19, 2018

Susurrus commented Jan 19, 2018

Thomasdezeeuw commented Jan 21, 2018

Thomasdezeeuw commented Feb 18, 2019