search

nix-rust / nix

Rust friendly bindings to *nix APIs
MIT License
2.63k stars 660 forks source link

Shouldn't TimeSpec::from_duration preserve monotonicity? #2479

Open sirhcel opened 3 weeks ago

sirhcel commented 3 weeks ago

When creating a TimeSpec from a std::time::Duration with more than libc::time_t seconds, the resulting value might be negative and will no longer preserve monotonicity with respect to the input duration. This happens to the integer casting in TimeSpec::from_duration.

See this example on Rust Playground.

I learned about this behavior through errors for negative timeouts from ppoll on Linux at runtime (https://github.com/serialport/serialport-rs/issues/207). I did not expect this for the "porcellain" type Duration and the docs did not hint at it. And likely other users of the "porcellain" as well. When manually converting to the "plumbing" typetime_t I wold have looked deeper into the nitty gritty integer casts and cared for this case by saturating.

So shouldn't TimeSpec::from_duration preserve monotonicity? Isn't an inaccurate but still large TimeSpec less surprising?

asomers commented 3 weeks ago

The problem is that a Unix Timespec can't represent times greater than i64::MAX seconds. Saturating the conversion would be bad, because then subtracting two TimeSpecs that were produced by TimeSpec::from_duration wouldn't be guaranteed to give the correct answer. Arguably, what Nix ought to do would be to provide TimeSpec::from_instant(i: std::time::Instant) instead of from_duration, because std::time::Instant actually uses a TimeSpec under the hood. And perhaps a TryFrom<Duration> impl, too.

sirhcel commented 3 weeks ago

The problem is that a Unix Timespec can't represent times greater than i64::MAX seconds. Saturating the conversion would be bad, because then subtracting two TimeSpecs that were produced by TimeSpec::from_duration wouldn't be guaranteed to give the correct answer.

This is a fair point against saturating.

And perhaps a TryFrom<Duration> impl, too.

What about transitioning towards this impl over some time? I could prepare PRs for adding TryFrom<Duration> and once this is released the successor for deprecating From<Duration> so that other young players could get safely around this.

asomers commented 3 weeks ago

What about transitioning towards this impl over some time? I could prepare PRs for adding TryFrom<Duration> and once this is released the successor for deprecating From<Duration> so that other young players could get safely around this.

Yes, that would be good.