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

Adding an implementation for timerfd. #1261

Merged
merged 1 commit into from Jul 24, 2020
Merged

Adding an implementation for timerfd. #1261

merged 1 commit into from Jul 24, 2020

Conversation

vdagonneau-anssi
Copy link
Contributor

Hello,

I have encountered a situation where I needed to use timerfd in addition to other fd with nix implementation of epoll.

Here is a basic implementation of this feature, I also added some documentation and tests.

Note: currently testing works by pausing for 1-3 seconds. Obviously it is pretty bad if we want tests to run quickly but that was my best idea at the time. Feel free to suggest any other way.

Thank you!

asomers-ax
asomers-ax suggested changes Jun 25, 2020
View reviewed changes
Copy link
Contributor

@asomers-ax asomers-ax left a comment

Choose a reason for hiding this comment

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

This looks like a good start. But I think the interface could be more Rusty. Instead of requiring the user to read into a buffer that will be ignored, can you add a sleep method or something that does the read?

test/sys/test_timerfd.rs Outdated Show resolved Hide resolved
test/sys/test_timerfd.rs Outdated Show resolved Hide resolved
src/sys/timerfd.rs Outdated Show resolved Hide resolved
src/sys/timerfd.rs Outdated Show resolved Hide resolved
src/sys/timerfd.rs Outdated Show resolved Hide resolved
src/sys/timerfd.rs Show resolved Hide resolved
test/sys/test_timerfd.rs Outdated Show resolved Hide resolved
@vdagonneau-anssi
Copy link
Contributor Author

I addressed most of your comments in the latest commit I think.

As for having a sleep method or equivalent, I think it is a fairly high level interface and I will need to think about how to implement it properly.

It is worth noting that reading on the fd has different meanings depending on how you created the timerfd in the first place (or modified it with fcntl). Specifically, the call to read can either be blocking or not.

If we indeed need a higher level interface then I would like to move away from the method names I have right now to make it clear that the semantics are different from the underlying syscalls.

asomers
asomers requested changes Jun 27, 2020
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

The changes look good. But I do think there needs to be a higher level interface than read. The goal is for the API to be as low-level as any Nix user will ever need, but no lower. Requiring the user to declare an empty buffer, for example, is too low.

src/sys/timerfd.rs Outdated Show resolved Hide resolved
src/sys/timerfd.rs Show resolved Hide resolved
@vdagonneau-anssi
Copy link
Contributor Author

I pushed a new set of changes including a slightly higher level interface.

You now have 5 methods for the TimerFd: new, set, get, unsetand wait.

The idea is that you use new to create a new timerfd, it is created without any expiration. You can then set a new expiration and get it back. If you wish you can also unset any previous expiration that has been set. Finally, you can "acknowledge"/"wait" the timer with wait.

I still feel somewhat uneasy about the wait name as it really doesn't make a lot of sense in the context of epoll.

Typically in the case of epoll, you would create a timerfd, set an expiration, feed it to epoll. Once the timer is triggered, epoll will notify you that the timerfd has become readable and you need to "acknowledge" it (a.k.a. read from it) otherwise epoll is going to keep notifying you about it.

The name wait works well in a conventional approach where you create the timerfd, set an expiration and the read from it. In that case wait will indeed wait until timerfd becomes readable.

asomers
asomers reviewed Jun 30, 2020
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Looks good! Could you please add a CHANGELOG entry, too?

@vdagonneau-anssi
Copy link
Contributor Author

Ok, I added it to the changelog.

@asomers
Copy link
Member

asomers commented Jul 3, 2020

Only one more problem: you'll have to rebase to fix the test failure.

@vdagonneau-anssi
Adding an implementation and some basic tests for timerfd.
f3bd6ed 
Removed support for timerfd on Android as it seems to have been deprecated? See https://android.googlesource.com/platform/development/+/73a5a3b/ndk/platforms/android-20/include/sys/timerfd.h or rust-lang/libc#1589

Removed the public status of `TimerSpec`, as it should not be exposed to the user.

Implemented `FromRawFd` for `TimerFd` as it already implements `AsRawFd`.

Addressed comments from the latest code review:
  - Removed upper bound assertions on timer expirations in tests.
  - Made the main example runnable and added code to show how to wait for the timer.
  - Refactored `ClockId` to use `libc_enum`.
  - Added comments for all public parts of the module.
  - Wrapped to 80 cols.
  - Changed the size of the buffer in the tests to the minimum required.

* Ran rustfmt.
* Added a `From` implementation for `libc::timespec` -> `TimeSpec`.
* Reworked the example with the new changes and changed the timer from 5 to 1 second.
* Added a constructor for a 0-initialized `TimerSpec`.
* Added a new method to get the timer configured expiration (based on timerfd_gettime).
* Added an helper method to unset the expiration of the timer.
* Added a `wait` method to actually read from the timer.
* Renamed `settime` into just `set`.
* Refactored the tests and added a new one that tests both the `unset` and the `get` method.

Modified CHANGELOG.
@vdagonneau-anssi
Copy link
Contributor Author

Ok, I rebased on top of master.

asomers
asomers approved these changes Jul 24, 2020
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

LGTM!

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 24, 2020

Build succeeded:

@bors bors bot merged commit b1ba074 into nix-rust:master Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers-ax asomers-ax asomers-ax requested changes

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@vdagonneau-anssi @asomers @asomers-ax