hmon: logic monitor post-review fixes

- Move `loom` imports to `common`.
- Remove `Sync` from `LogicMonitor`.
  - `PhantomUnsync` added to `common`.
- `LogicEvaluationError` -> replace `From` with `TryFrom`.
- Replace `monitor_status` and `set_monitor_status` type.
  - From `u8` to `LogicEvaluationError`.
- Replace `update` with `swap` in `LogicState`.

Author: arkjedrz

src/health_monitoring_lib/rust/common.rs

@@ -16,7 +16,9 @@ use crate::heartbeat::HeartbeatEvaluationError;
 use crate::log::ScoreDebug;
 use crate::logic::LogicEvaluationError;
 use crate::tag::MonitorTag;
+use core::cell::Cell;
 use core::hash::Hash;
+use core::marker::PhantomData;
 use core::time::Duration;
 use std::sync::Arc;
 use std::time::Instant;
@@ -151,6 +153,14 @@ where
     T::try_from(millis).expect("Duration is too big for the integer of this type")
 }
 
+/// Marker for disabling [`Sync`].
+pub(crate) type PhantomUnsync = PhantomData<Cell<()>>;
+
+#[cfg(not(loom))]
+pub use core::sync::atomic::{AtomicU64, Ordering};
+#[cfg(loom)]
+pub use loom::sync::atomic::{AtomicU64, Ordering};
+
 #[cfg(all(test, not(loom)))]
 mod tests {
     use crate::common::{duration_to_int, time_offset, TimeRange};

src/health_monitoring_lib/rust/heartbeat/heartbeat_state.rs

@@ -11,13 +11,9 @@
 // SPDX-License-Identifier: Apache-2.0
 // *******************************************************************************
 
+use crate::common::{AtomicU64, Ordering};
 use core::cmp::min;
 
-#[cfg(not(loom))]
-use core::sync::atomic::{AtomicU64, Ordering};
-#[cfg(loom)]
-use loom::sync::atomic::{AtomicU64, Ordering};
-
 /// Snapshot of a heartbeat state.
 /// Layout (u64) = | heartbeat timestamp: 62 bits | heartbeat counter: 2 bits |
 #[derive(Clone, Copy, Default)]

src/health_monitoring_lib/rust/logic/logic_monitor.rs

@@ -11,19 +11,20 @@
 // SPDX-License-Identifier: Apache-2.0
 // *******************************************************************************
 
-use crate::common::{Monitor, MonitorEvalHandle, MonitorEvaluationError, MonitorEvaluator};
+use crate::common::{Monitor, MonitorEvalHandle, MonitorEvaluationError, MonitorEvaluator, PhantomUnsync};
 use crate::log::{error, warn, ScoreDebug};
 use crate::logic::logic_state::LogicState;
 use crate::protected_memory::ProtectedMemoryAllocator;
 use crate::tag::{MonitorTag, StateTag};
 use crate::HealthMonitorError;
 use core::hash::Hash;
+use core::marker::PhantomData;
 use std::collections::HashMap;
 use std::sync::Arc;
 use std::time::Instant;
 
 /// Internal OK state representation.
-const OK_STATE: u8 = 0;
+pub(super) const OK_STATE: u8 = 0;
 
 /// Logic evaluation errors.
 #[repr(u8)]
@@ -33,6 +34,8 @@ pub enum LogicEvaluationError {
     InvalidState = OK_STATE + 1,
     /// Transition is invalid.
     InvalidTransition,
+    /// Unknown error.
+    UnmappedError,
 }
 
 impl From<LogicEvaluationError> for u8 {
@@ -41,12 +44,18 @@ impl From<LogicEvaluationError> for u8 {
     }
 }
 
-impl From<u8> for LogicEvaluationError {
-    fn from(value: u8) -> Self {
+impl TryFrom<u8> for LogicEvaluationError {
+    type Error = ();
+
+    fn try_from(value: u8) -> Result<Self, Self::Error> {
+        const INVALID_STATE: u8 = LogicEvaluationError::InvalidState as u8;
+        const INVALID_TRANSITION: u8 = LogicEvaluationError::InvalidTransition as u8;
+        const UNMAPPED_ERROR: u8 = LogicEvaluationError::UnmappedError as u8;
         match value {
-            value if value == LogicEvaluationError::InvalidState as u8 => LogicEvaluationError::InvalidState,
-            value if value == LogicEvaluationError::InvalidTransition as u8 => LogicEvaluationError::InvalidTransition,
-            _ => panic!("Invalid underlying value of logic evaluation error."),
+            INVALID_STATE => Ok(LogicEvaluationError::InvalidState),
+            INVALID_TRANSITION => Ok(LogicEvaluationError::InvalidTransition),
+            UNMAPPED_ERROR => Ok(LogicEvaluationError::UnmappedError),
+            _ => Err(()),
         }
     }
 }
@@ -157,12 +166,16 @@ impl LogicMonitorBuilder {
 /// Logic monitor.
 pub struct LogicMonitor {
     inner: Arc<LogicMonitorInner>,
+    _unsync: PhantomUnsync,
 }
 
 impl LogicMonitor {
     /// Create a new [`LogicMonitor`] instance.
     fn new(inner: Arc<LogicMonitorInner>) -> Self {
-        Self { inner }
+        Self {
+            inner,
+            _unsync: PhantomData,
+        }
     }
 
     /// Perform transition to a new state.
@@ -198,8 +211,7 @@ struct LogicMonitorInner {
 impl MonitorEvaluator for LogicMonitorInner {
     fn evaluate(&self, _hmon_starting_point: Instant, on_error: &mut dyn FnMut(&MonitorTag, MonitorEvaluationError)) {
         let snapshot = self.logic_state.snapshot();
-        if snapshot.monitor_status() != OK_STATE {
-            let error = LogicEvaluationError::from(snapshot.monitor_status());
+        if let Err(error) = snapshot.monitor_status() {
             warn!("Logic monitor error observed: {:?}", error);
             on_error(&self.monitor_tag, error.into());
         }
@@ -235,10 +247,10 @@ impl LogicMonitorInner {
 
     fn transition(&self, target_state: StateTag) -> Result<StateTag, LogicEvaluationError> {
         // Load current monitor state.
-        let snapshot = self.logic_state.snapshot();
+        let mut snapshot = self.logic_state.snapshot();
 
         // Disallow operation in erroneous state.
-        if snapshot.monitor_status() != OK_STATE {
+        if snapshot.monitor_status().is_err() {
             warn!("Current logic monitor state cannot be determined");
             return Err(LogicEvaluationError::InvalidState);
         }
@@ -254,20 +266,17 @@ impl LogicMonitorInner {
                 "Requested state transition is invalid: {:?} -> {:?}",
                 current_state_node.tag, target_state
             );
+
             let error = LogicEvaluationError::InvalidTransition;
-            let _ = self.logic_state.update(|mut current_state| {
-                current_state.set_monitor_status(error.into());
-                Some(current_state)
-            });
+            snapshot.set_monitor_status(error);
+            let _ = self.logic_state.swap(snapshot);
             return Err(error);
         }
 
         // Find index of target state, then change current state.
         let target_state_index = self.find_index_by_tag(target_state)?;
-        let _ = self.logic_state.update(|mut current_state| {
-            current_state.set_current_state_index(target_state_index);
-            Some(current_state)
-        });
+        snapshot.set_current_state_index(target_state_index);
+        let _ = self.logic_state.swap(snapshot);
 
         Ok(target_state)
     }
@@ -277,7 +286,7 @@ impl LogicMonitorInner {
         let snapshot = self.logic_state.snapshot();
 
         // Disallow operation in erroneous state.
-        if snapshot.monitor_status() != OK_STATE {
+        if snapshot.monitor_status().is_err() {
             warn!("Current logic monitor state cannot be determined");
             return Err(LogicEvaluationError::InvalidState);
         }

src/health_monitoring_lib/rust/logic/logic_state.rs

@@ -11,10 +11,9 @@
 // SPDX-License-Identifier: Apache-2.0
 // *******************************************************************************
 
-#[cfg(not(loom))]
-use core::sync::atomic::{AtomicU64, Ordering};
-#[cfg(loom)]
-use loom::sync::atomic::{AtomicU64, Ordering};
+use crate::common::{AtomicU64, Ordering};
+use crate::logic::logic_monitor::OK_STATE;
+use crate::logic::LogicEvaluationError;
 
 /// Snapshot of a logic state.
 /// Layout (u64) = | current state index: 56 bits | monitor status: u8 |
@@ -53,12 +52,17 @@ impl LogicStateSnapshot {
     /// Monitor status.
     /// - zero if healthy.
     /// - `LogicEvaluationError` if not.
-    pub fn monitor_status(&self) -> u8 {
-        (self.0 & STATUS_MASK) as u8
+    pub fn monitor_status(&self) -> Result<(), LogicEvaluationError> {
+        let value = (self.0 & STATUS_MASK) as u8;
+        if value == OK_STATE {
+            Ok(())
+        } else {
+            Err(value.try_into().map_err(|_| LogicEvaluationError::UnmappedError)?)
+        }
     }
 
     /// Set monitor status.
-    pub fn set_monitor_status(&mut self, value: u8) {
+    pub fn set_monitor_status(&mut self, value: LogicEvaluationError) {
         self.0 = (value as u64) | (self.0 & !STATUS_MASK);
     }
 }
@@ -85,26 +89,16 @@ impl LogicState {
         LogicStateSnapshot::from(self.0.load(Ordering::Acquire))
     }
 
-    /// Update the logic state using the provided closure.
-    /// Closure receives the current state and should return an [`Option`] containing a new state.
-    /// If [`None`] is returned then the state was not updated.
-    pub fn update<F: FnMut(LogicStateSnapshot) -> Option<LogicStateSnapshot>>(
-        &self,
-        mut f: F,
-    ) -> Result<LogicStateSnapshot, LogicStateSnapshot> {
-        self.0
-            .fetch_update(Ordering::AcqRel, Ordering::Acquire, |prev| {
-                let snapshot = LogicStateSnapshot::from(prev);
-                f(snapshot).map(|new_snapshot: LogicStateSnapshot| new_snapshot.as_u64())
-            })
-            .map(LogicStateSnapshot::from)
-            .map_err(LogicStateSnapshot::from)
+    /// Store a new [`LogicStateSnapshot`] and return the previous one.
+    pub fn swap(&self, new: LogicStateSnapshot) -> LogicStateSnapshot {
+        self.0.swap(new.as_u64(), Ordering::AcqRel).into()
     }
 }
 
 #[cfg(all(test, not(loom)))]
 mod tests {
     use crate::logic::logic_state::{LogicState, LogicStateSnapshot};
+    use crate::logic::LogicEvaluationError;
     use core::sync::atomic::Ordering;
 
     #[test]
@@ -114,7 +108,7 @@ mod tests {
 
         assert_eq!(state.as_u64(), (initial_state_index as u64) << u8::BITS);
         assert_eq!(state.current_state_index(), initial_state_index);
-        assert_eq!(state.monitor_status(), 0);
+        assert!(state.monitor_status().is_ok());
     }
 
     #[test]
@@ -123,16 +117,18 @@ mod tests {
 
         assert_eq!(state.as_u64(), 0x00);
         assert_eq!(state.current_state_index(), 0);
-        assert_eq!(state.monitor_status(), 0);
+        assert!(state.monitor_status().is_ok());
     }
 
     #[test]
     fn snapshot_from_u64_valid() {
-        let state = LogicStateSnapshot::from(0xDEADBEEF_DEADBEEF);
+        let state = LogicStateSnapshot::from(0xDEADBEEF_DEADBE01);
 
-        assert_eq!(state.as_u64(), 0xDEADBEEF_DEADBEEF);
-        assert_eq!(state.current_state_index(), 0xDEADBEEF_DEADBEEF >> u8::BITS);
-        assert_eq!(state.monitor_status(), 0xEF);
+        assert_eq!(state.as_u64(), 0xDEADBEEF_DEADBE01);
+        assert_eq!(state.current_state_index(), 0xDEADBEEF_DEADBE01 >> u8::BITS);
+        assert!(state
+            .monitor_status()
+            .is_err_and(|e| e == LogicEvaluationError::InvalidState));
     }
 
     #[test]
@@ -141,18 +137,20 @@ mod tests {
 
         assert_eq!(state.as_u64(), u64::MAX);
         assert_eq!(state.current_state_index(), (u64::MAX >> u8::BITS) as usize);
-        assert_eq!(state.monitor_status(), u8::MAX);
+        assert!(state
+            .monitor_status()
+            .is_err_and(|e| e == LogicEvaluationError::UnmappedError));
     }
 
     #[test]
     fn snapshot_set_current_state_index_valid() {
-        let mut state = LogicStateSnapshot::from(0xDEADBEEF_DEADBEEF);
+        let mut state = LogicStateSnapshot::from(0xDEADBEEF_DEADBE00);
         state.set_current_state_index(0x00CAFEBA_DCAFEBAD);
 
         assert_eq!(state.current_state_index(), 0x00CAFEBA_DCAFEBAD);
 
         // Check other parameters unchanged.
-        assert_eq!(state.monitor_status(), 0xEF);
+        assert!(state.monitor_status().is_ok());
     }
 
     #[test]
@@ -165,9 +163,11 @@ mod tests {
     #[test]
     fn snapshot_set_monitor_status_valid() {
         let mut state = LogicStateSnapshot::from(0xDEADBEEF_DEADBEEF);
-        state.set_monitor_status(0xFA);
+        state.set_monitor_status(LogicEvaluationError::InvalidTransition);
 
-        assert_eq!(state.monitor_status(), 0xFA);
+        assert!(state
+            .monitor_status()
+            .is_err_and(|e| e == LogicEvaluationError::InvalidTransition));
 
         // Check other parameters unchanged.
         assert_eq!(state.current_state_index(), 0xDEADBEEF_DEADBEEF >> u8::BITS);
@@ -191,35 +191,19 @@ mod tests {
     }
 
     #[test]
-    fn state_update_some() {
+    fn state_swap() {
         let state = LogicState::new(0);
-        let _ = state.update(|prev_snapshot| {
-            // Make sure state is as expected.
-            assert_eq!(prev_snapshot.as_u64(), 0x00);
-            assert_eq!(prev_snapshot.current_state_index(), 0);
-            assert_eq!(prev_snapshot.monitor_status(), 0);
-
-            Some(LogicStateSnapshot::from(0xDEADBEEF_DEADBEEF))
-        });
-
-        let _ = state.update(|prev_snapshot| {
-            // Make sure state is as expected.
-            assert_eq!(prev_snapshot.as_u64(), 0xDEADBEEF_DEADBEEF);
-            assert_eq!(prev_snapshot.current_state_index(), 0xDEADBEEF_DEADBEEF >> u8::BITS);
-            assert_eq!(prev_snapshot.monitor_status(), 0xEF);
-
-            Some(LogicStateSnapshot::from(0))
-        });
-
-        assert_eq!(state.snapshot().as_u64(), 0);
-    }
-
-    #[test]
-    fn state_update_none() {
-        let state = LogicState::new(4321);
-        let _ = state.update(|_| Some(LogicStateSnapshot::from(0xDEADBEEF_DEADBEEF)));
-        let _ = state.update(|_| None);
-
-        assert_eq!(state.snapshot().as_u64(), 0xDEADBEEF_DEADBEEF);
+        let prev_snapshot = state.swap(LogicStateSnapshot::from(0xDEADBEEF_DEADBE02));
+
+        assert_eq!(prev_snapshot.as_u64(), 0x00);
+        assert_eq!(prev_snapshot.current_state_index(), 0);
+        assert!(prev_snapshot.monitor_status().is_ok());
+
+        let curr_snapshot = state.snapshot();
+        assert_eq!(curr_snapshot.as_u64(), 0xDEADBEEF_DEADBE02);
+        assert_eq!(curr_snapshot.current_state_index(), 0xDEADBEEF_DEADBE02 >> u8::BITS);
+        assert!(curr_snapshot
+            .monitor_status()
+            .is_err_and(|e| e == LogicEvaluationError::InvalidTransition));
     }
 }