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

Jump to bottom

Introduce timer_* support #1622

Merged
merged 1 commit into from Dec 31, 2021
Merged

Introduce timer_* support #1622

merged 1 commit into from Dec 31, 2021

Conversation

blt
Copy link
Contributor

@blt blt commented Dec 28, 2021 •
edited

This commit adds support for the signal timer mechanism in POSIX, the mirror to timerfd on Linux. I wasn't quite sure of how to fit into the project organization but hopefully this patch isn't too far off.

Resolves #1424

Signed-off-by: Brian L. Troutwine brian@troutwine.us

@blt blt force-pushed the timers branch 4 times, most recently from 6edeef3 to 7f9da7d Compare December 28, 2021 01:55
asomers
asomers requested changes Dec 28, 2021
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.

It's a good start, especially the documentation. In addition to the inline comments, could you please add some tests and an example?

CHANGELOG.md Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/sys/mod.rs Outdated Show resolved Hide resolved
src/sys/signal.rs Outdated Show resolved Hide resolved
src/sys/time.rs Outdated Show resolved Hide resolved
src/sys/timer.rs Outdated Show resolved Hide resolved
src/sys/timer.rs Outdated Show resolved Hide resolved
src/sys/timer.rs Outdated Show resolved Hide resolved
src/sys/timer.rs Outdated Show resolved Hide resolved
src/sys/timer.rs Outdated Show resolved Hide resolved
@blt blt force-pushed the timers branch 5 times, most recently from c557721 to b55d254 Compare December 28, 2021 21:25
@blt blt requested a review from asomers December 28, 2021 21:25
@blt
Copy link
Contributor Author

blt commented Dec 28, 2021

It's a good start, especially the documentation. In addition to the inline comments, could you please add some tests and an example?

I believe this is ready for re-review. I've expanded OS coverage to include everything libc covers and added a no_run example to the timer module. With regard to tests, I'm not totally sure what's approachable with unit tests in the things I've added here, but if you have anything you'd like to see covered I'd be glad to add it.

asomers
asomers requested changes Dec 29, 2021
View reviewed changes
src/sys/signal.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/sys/timer.rs Outdated Show resolved Hide resolved
@blt blt requested a review from asomers December 29, 2021 04:38
@blt
Copy link
Contributor Author

blt commented Dec 29, 2021

I believe this is ready again. I did manage to tidy up the cfg mess in Timer::new, happily.

asomers
asomers requested changes Dec 30, 2021
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.

Did you ever try to write a test? Something like test_alarm in test/test_unistd.rs might be a good template to follow.

src/sys/mod.rs Show resolved Hide resolved
src/sys/time.rs Outdated Show resolved Hide resolved
src/sys/time.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@blt
Copy link
Contributor Author

blt commented Dec 30, 2021

Did you ever try to write a test? Something like test_alarm in test/test_unistd.rs might be a good template to follow.

I didn't, no, as mentioned here I was open to suggestions considering the low surface area introduced in this patch. I'll look at what's in test/test_unistd.rs, thanks.

@blt blt force-pushed the timers branch 3 times, most recently from d0fa578 to e28c074 Compare December 30, 2021 05:45
@blt blt requested a review from asomers December 30, 2021 05:58
@blt
Copy link
Contributor Author

blt commented Dec 30, 2021

Aside from this outstanding thread I've incorporated feedback and think this is ready for review again. Of note I've added a tests/test_timer that is patterned on the example program in the timer docs and test_alarm from test_unistd.

asomers
asomers reviewed Dec 31, 2021
View reviewed changes
test/test_timer.rs Outdated Show resolved Hide resolved
@blt
Introduce timer_* support
77febe0 
This commit adds support for the signal timer mechanism in POSIX, the
mirror to timerfd on Linux.

Resolves nix-rust#1424

Signed-off-by: Brian L. Troutwine <brian@troutwine.us>
@blt blt requested a review from asomers December 31, 2021 04:41
asomers
asomers approved these changes Dec 31, 2021
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.

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 31, 2021

Build succeeded!

And happy new year! 🎉

@bors bors bot merged commit ee0543f into nix-rust:master Dec 31, 2021
@blt blt deleted the timers branch January 3, 2022 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.

Add timer_* bindings
2 participants
@blt @asomers