Add alarm #830

Thomasdezeeuw · 2018-01-07T21:10:49Z

Fixed #828.

asomers · 2018-01-08T04:30:48Z

bors r+

830: Add alarm r=asomers a=Thomasdezeeuw Fixed #828.

bors · 2018-01-08T05:13:54Z

Build failed

continuous-integration/travis-ci/push

asomers · 2018-01-08T05:31:54Z

Travis failed because of a temporary network failure. Try again.

bors r+

830: Add alarm r=asomers a=Thomasdezeeuw Fixed #828.

bors · 2018-01-08T10:01:55Z

Timed out

Thomasdezeeuw · 2018-01-09T10:50:15Z

Bors timeout out, but the build passed. Does that mean it's ok to merge?

asomers · 2018-01-11T04:11:36Z

The timeout was probably buildbot's fault. Buildbot has suddenly started to run much slower than it used to. There's probably some kind of problem with the VM host. We should just try again. However, you'll have to rebase to fix the conflict in CHANGELOG.md.

Thomasdezeeuw · 2018-01-11T14:18:15Z

@asomers This should be good to merge now.

Susurrus · 2018-01-28T03:16:52Z

You'll need to rebase this to update the CHANGELOG now that 0.10.0 was released.

Thomasdezeeuw · 2018-01-29T15:19:23Z

done.

Susurrus · 2018-01-30T06:36:22Z

In looking this over again I realized that this is another one of those functions that overloads the intneger range with different functionality. I'm not a fan of this and generally try to alter the API to avoid this. For example in this case there are really two functions that should exist, one to set an alarm to a given number of seconds and another to clear any existing alarm. I'd much prefer using an enum datatype for the arguments so you'd do something like alarm(Seconds(15)) to set the alarm and alarm(Clear) to clear it. That reads better to me than alarm(15) and alarm(0).

Thomasdezeeuw · 2018-01-30T11:38:06Z

@surrus I've change the argument to SetAlarm, with the variants Cancel and Set to be more inline with the documentation on Opengroup (they use cancel rather then clear).

Susurrus · 2018-01-30T16:48:12Z

src/unistd.rs

+/// use nix::unistd::{alarm, SetAlarm};
+///
+/// // Set an alarm for 60 seconds from now.
+/// alarm(SetAlarm::Set(60));


I forgot that you need to specify the enum name when you initialize a variant of it, so this doesn't read as well as I hoped. I was originally going to suggest that instead of an enum we split alarm into two functions, alarm_set(c_uint) and alarm_cancel(). I'm now thinking that would be a more ergonomic API. The only downside to that is discoverability, but I think the docs will be enough there. And if a user searches the docs for "alarm", I'd expect them to find these two functions and be able to understand their usage.

@Thomasdezeeuw What do you think?

In my original pr (#828) I had two those function exactly, but @asomers was against that.

I didn't realize this was a spin off of an earlier PR, sorry about that. Read through it quickly and it provides a little more context. I didn't get a chance to respond early in the last PR, but I think if we have functions that start with alarm_ it's a nice API. So let's go ahead and switch to that type of API. @asomers Care to chime in here?

I'm also actually thinking rather than name these functions that are split off from a single libc function by prefixing them within their function name, instead prefix them within a module. So you'd have alarm::set(c_uint) and alarm::cancel(). That should really help with the end-user documentation. We're already doing this with ptrace.

Actually, I like the module idea. It's less verbose than using enums, but still Rustier than using alarm_set and alarm_cancel.

I've put the function into their own module and also expanded the documentation.

Susurrus

Awesome work with the examples and the tests, I really like how this is shaping up! Some small grammatical fixes and a couple of API questions to finish up.

Susurrus · 2018-01-31T16:54:56Z

src/unistd.rs

+    //!
+    //! # Examples
+    //!
+    //! Canceling an alarm.


: instead of .

Susurrus · 2018-01-31T16:55:01Z

src/unistd.rs

+    //!
+    //! # Notes
+    //!
+    //! Scheduling an alarm will trigger a `SIGALRM` signal when the the time


Susurrus · 2018-01-31T16:55:25Z

src/unistd.rs

+    //!
+    //! See also [alarm](http://pubs.opengroup.org/onlinepubs/9699919799/functions/alarm.html).
+    //!
+    //! # Notes


I would just remove this "Notes" header

Susurrus · 2018-01-31T16:55:36Z

src/unistd.rs

+    //! assert_eq!(alarm::cancel(), 60);
+    //! ```
+    //!
+    //! Scheduling an alarm and waiting for the signal.


: instead of .

Susurrus · 2018-01-31T16:56:01Z

src/unistd.rs

+    /// Schedule an alarm signal.
+    ///
+    /// This will cause the system to generate a `SIGALRM` signal for the
+    /// process after the number of `secs` have elapsed.


"after the specified number"

Susurrus · 2018-01-31T16:56:39Z

src/unistd.rs

+    /// This will cause the system to generate a `SIGALRM` signal for the
+    /// process after the number of `secs` have elapsed.
+    ///
+    /// Returns the leftover time of a previously set alarm, or `0` if no alarm


Do you think an enum return type would be more helpful here than just an int?

I was thinking about that as well, maybe Option<libc::c_uint> with 0 mapped to None.

I think using the Option type here makes a lot of sense!

Susurrus · 2018-01-31T16:57:16Z

src/unistd.rs

+    ///
+    /// Returns the leftover time of a previously set alarm, or `0` if no alarm
+    /// was set.
+    pub fn set(secs: libc::c_uint) -> libc::c_uint {


Should we check for 0 in the input argument with an assertion? I think if people are doing that they should really be using cancel instead of set.

I could add a simple assertion.

What about this:

assert!(secs != 0, "passing 0 to `alarm::set` is not allowed, to cancel an alarm use `alarm::cancel`");

Sure, that'd be fine.

Susurrus

Minor doc edits then it LGTM.

Susurrus · 2018-02-01T04:26:31Z

src/unistd.rs

+    /// This will cause the system to generate a `SIGALRM` signal for the
+    /// process after the specified number of seconds have elapsed.
+    ///
+    /// Returns the leftover time of a previously set alarm, or `0` if no alarm


Change to "Returns the leftover time of a previously set alarm if there was one" since now this returns an Option.

Susurrus · 2018-02-01T04:26:40Z

src/unistd.rs

+
+    /// Cancel an previously set alarm signal.
+    ///
+    /// Returns the leftover time of a previously set alarm, or 0 if no alarm


Same as the above.

Susurrus · 2018-02-01T04:36:27Z

src/unistd.rs

+pub mod alarm {
+    //! Alarm signal scheduling.
+    //!
+    //! See also [alarm](http://pubs.opengroup.org/onlinepubs/9699919799/functions/alarm.html).


Change the link to "alarm(2)" and move this into its own "# References" section after the Examples section.

Susurrus · 2018-02-01T04:36:42Z

CHANGELOG.md

@@ -7,6 +7,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).

 ### Added

+- Added `alarm`.


Just make this one line.

Susurrus · 2018-02-02T05:58:34Z

LGTM. Please squash into a single commit and then we'll merge.

@asomers Care to take another gander?

This module has two functions; set: set an alarm, and cancel: cancels a previously set alarm.

Thomasdezeeuw · 2018-02-02T11:28:10Z

@Susurrus done.

asomers · 2018-02-03T17:44:28Z

bors r+

830: Add alarm r=asomers a=Thomasdezeeuw Fixed #828.

bors · 2018-02-03T18:09:09Z

Build succeeded

Thomasdezeeuw mentioned this pull request Jan 7, 2018

Add alarm and cancel_alarm #828

Closed

bors bot added a commit that referenced this pull request Jan 8, 2018

Merge #830

f957cad

830: Add alarm r=asomers a=Thomasdezeeuw Fixed #828.

bors bot added a commit that referenced this pull request Jan 8, 2018

Merge #830

7c21401

830: Add alarm r=asomers a=Thomasdezeeuw Fixed #828.

Susurrus reviewed Jan 30, 2018

View reviewed changes

Susurrus suggested changes Jan 31, 2018

View reviewed changes

Susurrus suggested changes Feb 1, 2018

View reviewed changes

Susurrus approved these changes Feb 2, 2018

View reviewed changes

Add alarm module

fecf484

This module has two functions; set: set an alarm, and cancel: cancels a previously set alarm.

asomers approved these changes Feb 3, 2018

View reviewed changes

bors bot added a commit that referenced this pull request Feb 3, 2018

Merge #830

60aaa0d

830: Add alarm r=asomers a=Thomasdezeeuw Fixed #828.

bors bot merged commit fecf484 into nix-rust:master Feb 3, 2018

SteveLauC mentioned this pull request Nov 29, 2023

FreeBSD's rfork wrapper proposal #2121

Merged

		@@ -7,6 +7,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).

		### Added

		- Added `alarm`.

Add alarm #830

Add alarm #830

Conversation

Thomasdezeeuw commented Jan 7, 2018

asomers commented Jan 8, 2018

bors bot commented Jan 8, 2018

Build failed

asomers commented Jan 8, 2018

bors bot commented Jan 8, 2018

Timed out

Thomasdezeeuw commented Jan 9, 2018

asomers commented Jan 11, 2018

Thomasdezeeuw commented Jan 11, 2018

Susurrus commented Jan 28, 2018

Thomasdezeeuw commented Jan 29, 2018

Susurrus commented Jan 30, 2018

Thomasdezeeuw commented Jan 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus Jan 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus left a 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

Susurrus commented Feb 2, 2018

Thomasdezeeuw commented Feb 2, 2018

asomers commented Feb 3, 2018

bors bot commented Feb 3, 2018

Build succeeded

Susurrus Jan 31, 2018 •

edited

Loading