From 077d6d28a45ecddc88b45cb81bab0b14ee4d2529 Mon Sep 17 00:00:00 2001 From: Christian Meusel Date: Tue, 15 Oct 2024 15:51:25 +0200 Subject: [PATCH 1/4] Switch to TryFrom conversions for TimeSpec and Duration Their underlying types are not congruent and therefor "lossy" conversions with usually unexpected behavior result from it. Let's make this explicit by switching from From conversions to TryFrom. The error types used there are quite spartan. Is there a need for more detailed reporting? --- src/sys/time.rs | 53 ++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 41 insertions(+), 12 deletions(-) diff --git a/src/sys/time.rs b/src/sys/time.rs index af436cabd5..7e9875955e 100644 --- a/src/sys/time.rs +++ b/src/sys/time.rs @@ -209,15 +209,41 @@ impl From for TimeSpec { } } -impl From for TimeSpec { - fn from(duration: Duration) -> Self { - Self::from_duration(duration) +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct TryFromDurationError; + +impl TryFrom for TimeSpec { + type Error = TryFromDurationError; + + fn try_from(duration: Duration) -> Result { + Self::try_from_duration(duration) } } -impl From for Duration { - fn from(timespec: TimeSpec) -> Self { - Duration::new(timespec.0.tv_sec as u64, timespec.0.tv_nsec as u32) +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct TryFromTimeSpecError; + +impl TryFrom for Duration { + type Error = TryFromTimeSpecError; + + fn try_from(timespec: TimeSpec) -> Result { + let secs = timespec + .0 + .tv_sec + .try_into() + .map_err(|_| TryFromTimeSpecError)?; + let nanos = timespec + .0 + .tv_nsec + .try_into() + .map_err(|_| TryFromTimeSpecError)?; + + // Error out here rather than letting Duration::new panic. + if nanos > Duration::MAX.subsec_nanos() { + return Err(TryFromTimeSpecError); + } + + Ok(Duration::new(secs, nanos)) } } @@ -365,13 +391,16 @@ impl TimeSpec { self.0.tv_nsec } - #[cfg_attr(target_env = "musl", allow(deprecated))] - // https://github.com/rust-lang/libc/issues/1848 - pub const fn from_duration(duration: Duration) -> Self { + pub fn try_from_duration( + duration: Duration, + ) -> Result { let mut ts = zero_init_timespec(); - ts.tv_sec = duration.as_secs() as time_t; - ts.tv_nsec = duration.subsec_nanos() as timespec_tv_nsec_t; - TimeSpec(ts) + ts.tv_sec = duration + .as_secs() + .try_into() + .map_err(|_| TryFromDurationError)?; + ts.tv_nsec = duration.subsec_nanos().into(); + Ok(TimeSpec(ts)) } pub const fn from_timespec(timespec: timespec) -> Self { From 404aa68f0f1e4d737ae54f7fff8fafc70671715f Mon Sep 17 00:00:00 2001 From: Christian Meusel Date: Tue, 15 Oct 2024 15:55:25 +0200 Subject: [PATCH 2/4] Use TryFrom in tests for TimeSpec --- test/sys/test_time.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/sys/test_time.rs b/test/sys/test_time.rs index 0510225a92..82a2ef9826 100644 --- a/test/sys/test_time.rs +++ b/test/sys/test_time.rs @@ -15,12 +15,12 @@ pub fn test_timespec() { } #[test] -pub fn test_timespec_from() { +pub fn test_timespec_try_from() { let duration = Duration::new(123, 123_456_789); let timespec = TimeSpec::nanoseconds(123_123_456_789); - assert_eq!(TimeSpec::from(duration), timespec); - assert_eq!(Duration::from(timespec), duration); + assert_eq!(TimeSpec::try_from(duration).unwrap(), timespec); + assert_eq!(Duration::try_from(timespec).unwrap(), duration); } #[test] From d6307186c5bd46d2d3c2b94d92102df7a705c1e4 Mon Sep 17 00:00:00 2001 From: Christian Meusel Date: Sun, 20 Oct 2024 23:54:45 +0200 Subject: [PATCH 3/4] Use TryFrom in tests using TimeSpec --- src/sys/timer.rs | 2 +- test/sys/test_socket.rs | 15 +++++++++------ test/sys/test_timer.rs | 2 +- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/sys/timer.rs b/src/sys/timer.rs index 8876f56972..8de4c06dc2 100644 --- a/src/sys/timer.rs +++ b/src/sys/timer.rs @@ -38,7 +38,7 @@ //! }); //! //! let mut timer = Timer::new(clockid, sigevent).unwrap(); -//! let expiration = Expiration::Interval(Duration::from_millis(250).into()); +//! let expiration = Expiration::Interval(Duration::from_millis(250).try_into().unwrap()); //! let flags = TimerSetTimeFlags::empty(); //! timer.set(expiration, flags).expect("could not set timer"); //! diff --git a/test/sys/test_socket.rs b/test/sys/test_socket.rs index 93aa606822..dcb6d8464b 100644 --- a/test/sys/test_socket.rs +++ b/test/sys/test_socket.rs @@ -69,7 +69,7 @@ pub fn test_timestamping() { } else { sys_time - ts }; - assert!(std::time::Duration::from(diff).as_secs() < 60); + assert!(std::time::Duration::try_from(diff).unwrap().as_secs() < 60); } #[cfg(target_os = "freebsd")] @@ -131,7 +131,7 @@ pub fn test_timestamping_realtime() { } else { sys_time - ts }; - assert!(std::time::Duration::from(diff).as_secs() < 60); + assert!(std::time::Duration::try_from(diff).unwrap().as_secs() < 60); } #[cfg(target_os = "freebsd")] @@ -189,7 +189,7 @@ pub fn test_timestamping_monotonic() { ::nix::time::clock_gettime(::nix::time::ClockId::CLOCK_MONOTONIC) .unwrap(); let diff = sys_time - ts; // Monotonic clock sys_time must be greater - assert!(std::time::Duration::from(diff).as_secs() < 60); + assert!(std::time::Duration::try_from(diff).unwrap().as_secs() < 60); } #[test] @@ -2985,7 +2985,7 @@ pub fn test_txtime() { let iov1 = [std::io::IoSlice::new(&sbuf)]; let now = clock_gettime(ClockId::CLOCK_MONOTONIC).unwrap(); - let delay = std::time::Duration::from_secs(1).into(); + let delay = std::time::Duration::from_secs(1).try_into().unwrap(); let txtime = (now + delay).num_nanoseconds() as u64; let cmsg = ControlMessage::TxTime(&txtime); @@ -3099,7 +3099,8 @@ fn test_recvmm2() -> nix::Result<()> { let mut data = MultiHeaders::<()>::preallocate(recv_iovs.len(), Some(cmsg)); - let t = TimeSpec::from_duration(std::time::Duration::from_secs(10)); + let t = TimeSpec::try_from_duration(std::time::Duration::from_secs(10)) + .unwrap(); let recv = recvmmsg( rsock.as_raw_fd(), @@ -3125,7 +3126,9 @@ fn test_recvmm2() -> nix::Result<()> { } else { sys_time - ts }; - assert!(std::time::Duration::from(diff).as_secs() < 60); + assert!( + std::time::Duration::try_from(diff).unwrap().as_secs() < 60 + ); #[cfg(not(any(qemu, target_arch = "aarch64")))] { saw_time = true; diff --git a/test/sys/test_timer.rs b/test/sys/test_timer.rs index ffd146867b..7222567996 100644 --- a/test/sys/test_timer.rs +++ b/test/sys/test_timer.rs @@ -50,7 +50,7 @@ fn alarm_fires() { }); let mut timer = Timer::new(clockid, sigevent).expect("failed to create timer"); - let expiration = Expiration::Interval(TIMER_PERIOD.into()); + let expiration = Expiration::Interval(TIMER_PERIOD.try_into().unwrap()); let flags = TimerSetTimeFlags::empty(); timer.set(expiration, flags).expect("could not set timer"); From 65b0fc4d8b77b9c9b25b76538908443ce2c43c3f Mon Sep 17 00:00:00 2001 From: Christian Meusel Date: Mon, 21 Oct 2024 00:35:21 +0200 Subject: [PATCH 4/4] Use fallible conversion for tv_nsec in TimeSpec --- src/sys/time.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/sys/time.rs b/src/sys/time.rs index 7e9875955e..4eb55f5bc7 100644 --- a/src/sys/time.rs +++ b/src/sys/time.rs @@ -391,6 +391,7 @@ impl TimeSpec { self.0.tv_nsec } + #[allow(clippy::unnecessary_fallible_conversions)] pub fn try_from_duration( duration: Duration, ) -> Result { @@ -399,7 +400,12 @@ impl TimeSpec { .as_secs() .try_into() .map_err(|_| TryFromDurationError)?; - ts.tv_nsec = duration.subsec_nanos().into(); + // There are targets with tv_nsec being i32. Use the fallible conversion for all targets as + // we are returning a Result due to the previous conversion anyway. + ts.tv_nsec = duration + .subsec_nanos() + .try_into() + .map_err(|_| TryFromDurationError)?; Ok(TimeSpec(ts)) }