orchestrator: Make invalid table entries unconstructible Every schema check is per-device, so the constructors can run them all. BootCheckpoint::new and DeviceConfig::new are const fn -- board tables still build in const context, so a bad table is still a build error -- but the fields are private now, and a checkpoint or device entry that violates the schema cannot be constructed at all. The free validate() is gone with the loophole it carried: it had to be remembered, and a board table that dropped the const fence compiled fine while broken. Construction is the one gate every entry passes. Board-local checks keep the const-fence pattern (validate_signals in the mock table), reading through the new accessors. Assisted-by: Claude:claude-fable-5 Signed-off-by: Christina Quast <christina.quast@9elements.com>
diff --git a/services/orchestrator/config/src/lib.rs b/services/orchestrator/config/src/lib.rs index c544a79..f4e7424 100644 --- a/services/orchestrator/config/src/lib.rs +++ b/services/orchestrator/config/src/lib.rs
@@ -4,6 +4,11 @@ //! Schema for the per-board device table. Board device tables //! (`target/<board>/devices.rs`) declare the values; no concrete line or //! device is named here. +//! +//! Invariants are enforced in the `const fn` constructors, so an invalid +//! table is a build error and there is no validate step to forget. Checks +//! on board-defined types belong next to the table that gives them +//! meaning (`target/mock/devices.rs` shows the pattern). #![cfg_attr(not(test), no_std)] @@ -12,21 +17,57 @@ /// re-resets the device and re-runs the whole walk, so budgets are /// per boot attempt and owned by the orchestrator state machine. /// -/// `signal` is a board-defined id — the schema attaches no meaning to it -/// and names no signal kinds. Each board defines its own vocabulary (a +/// The signal is a board-defined id — the schema attaches no meaning to +/// it and names no signal kinds. Each board defines its own vocabulary (a /// small enum: a GPIO line, a progress-register threshold, a message-path /// readiness) and gives it meaning in its `EvidenceReader`. The id is a /// defunctionalized evidence check: data in the table instead of a /// function, so the table stays printable, comparable, const-checkable — /// and could one day be generated instead of written. +/// +/// Fields are private so a checkpoint that violates the schema is +/// unrepresentable: [`new`](Self::new) is the only way in, and it checks. #[derive(Debug, Clone, Copy)] pub struct BootCheckpoint<G> { + name: &'static str, + signal: G, + timeout: core::time::Duration, +} + +impl<G> BootCheckpoint<G> { + /// Declares a checkpoint. `const`, so board tables run the checks at + /// build time. + /// + /// # Panics + /// + /// Panics — a build error in const context — if `name` is empty or + /// `timeout` is zero. + #[must_use] + pub const fn new(name: &'static str, signal: G, timeout: core::time::Duration) -> Self { + assert!(!name.is_empty(), "checkpoint name must not be empty"); + assert!(!timeout.is_zero(), "checkpoint timeout must not be zero"); + Self { + name, + signal, + timeout, + } + } + /// Names the checkpoint in failure reports ("bl1", "kernel", …). - pub name: &'static str, + /// Unique within a device's checkpoint list. + #[must_use] + pub const fn name(&self) -> &'static str { + self.name + } + /// Board-defined signal id, resolved by the board's `EvidenceReader` /// (in `orchestrator-capabilities`). An id rather than a function, so /// the table stays pure data — the type-level docs say why. - pub signal: G, + #[must_use] + pub const fn signal(&self) -> &G { + &self.signal + } + /// Window for one attempt at this checkpoint. Expiry is the boot /// walk's own judgment; hung devices report nothing. /// @@ -34,7 +75,10 @@ /// clockless. The walk consumes the windows and reports expiry as a /// failed attempt; a component's whole boot timeout is nothing more /// than its walk over these windows, in order. - pub timeout: core::time::Duration, + #[must_use] + pub const fn timeout(&self) -> core::time::Duration { + self.timeout + } } /// One managed downstream device, as declared by the board config. @@ -44,67 +88,79 @@ /// implementation) and its boot-signal vocabulary `G`, for the same /// reason: signal ids are board-specific. /// -/// Intentionally exhaustive (not `#[non_exhaustive]`): board tables -/// construct this struct by literal, which the attribute would forbid. -/// Adding a field is a breaking change that updates every board table. -/// /// Deliberately says nothing about attestation or commit requirements: /// those follow from what kind of device this is (iRoT-backed or /// symbiont, the orchestrator's `ComponentKind`), not from a table /// setting — a second knob would only let the two disagree. +/// +/// Fields are private so a device entry that violates the schema is +/// unrepresentable: [`new`](Self::new) is the only way in, and it checks. #[derive(Debug, Clone, Copy)] pub struct DeviceConfig<R, G: 'static> { - pub name: &'static str, - /// Reset signal id, passed to HalBootControl::new. - pub reset_signal: R, - /// Boot checkpoints, in the order the device passes them. The device - /// counts as booted when the last one is reached; a checkpoint whose - /// window expires fails the attempt — whether to retry or recover is - /// the orchestrator's decision, not table data. - pub checkpoints: &'static [BootCheckpoint<G>], + name: &'static str, + reset_signal: R, + checkpoints: &'static [BootCheckpoint<G>], } -/// Checks a device table. Board configs call this in a const context so a -/// bad table fails the build. -/// -/// Only schema-shape checks are possible here; checks on the board's own -/// types (signal ranges, uniqueness of signal ids) belong next to the -/// table that defines their meaning, in a board-local `const fn` run -/// alongside this one — `target/mock/devices.rs` shows the pattern. -pub const fn validate<R, G>(devices: &[DeviceConfig<R, G>]) { - let mut i = 0; - while i < devices.len() { - assert!(!devices[i].name.is_empty(), "device name must not be empty"); +impl<R, G> DeviceConfig<R, G> { + /// Declares a managed device. `const`, so board tables run the checks + /// at build time. + /// + /// # Panics + /// + /// Panics — a build error in const context — if `name` is empty, if + /// `checkpoints` is empty, or if two checkpoints share a name + /// (failure reports identify a checkpoint by name; a duplicate would + /// make them ambiguous). + #[must_use] + pub const fn new( + name: &'static str, + reset_signal: R, + checkpoints: &'static [BootCheckpoint<G>], + ) -> Self { + assert!(!name.is_empty(), "device name must not be empty"); assert!( - !devices[i].checkpoints.is_empty(), + !checkpoints.is_empty(), "device must declare at least one boot checkpoint" ); let mut c = 0; - while c < devices[i].checkpoints.len() { - assert!( - !devices[i].checkpoints[c].name.is_empty(), - "checkpoint name must not be empty" - ); - assert!( - !devices[i].checkpoints[c].timeout.is_zero(), - "checkpoint timeout must not be zero" - ); - // Failure reports identify a checkpoint by name; a duplicate - // would make them ambiguous. + while c < checkpoints.len() { let mut d = c + 1; - while d < devices[i].checkpoints.len() { + while d < checkpoints.len() { assert!( - !str_eq( - devices[i].checkpoints[c].name, - devices[i].checkpoints[d].name - ), + !str_eq(checkpoints[c].name, checkpoints[d].name), "checkpoint names must be unique per device" ); d += 1; } c += 1; } - i += 1; + Self { + name, + reset_signal, + checkpoints, + } + } + + /// The device's name in reports and logs. + #[must_use] + pub const fn name(&self) -> &'static str { + self.name + } + + /// Reset signal id, passed to HalBootControl::new. + #[must_use] + pub const fn reset_signal(&self) -> &R { + &self.reset_signal + } + + /// Boot checkpoints, in the order the device passes them. The device + /// counts as booted when the last one is reached; a checkpoint whose + /// window expires fails the attempt — whether to retry or recover is + /// the orchestrator's decision, not table data. + #[must_use] + pub const fn checkpoints(&self) -> &'static [BootCheckpoint<G>] { + self.checkpoints } } @@ -129,79 +185,56 @@ use super::*; use core::time::Duration; - // Board tables run validate() at compile time, where a rejection is a - // build error nobody can assert on. These tests call it at runtime to - // prove the reject paths actually fire — a vacuous loop would pass - // every `const _` check silently. + // Board tables run the constructors at compile time, where a + // rejection is a build error nobody can assert on. These tests call + // them at runtime to prove the reject paths actually fire. - const CHECKPOINT: BootCheckpoint<u8> = BootCheckpoint { - name: "boot-complete", - signal: 0, - timeout: Duration::from_secs(1), - }; + const CHECKPOINT: BootCheckpoint<u8> = + BootCheckpoint::new("boot-complete", 0, Duration::from_secs(1)); - const DEVICE: DeviceConfig<u8, u8> = DeviceConfig { - name: "dev", - reset_signal: 0, - checkpoints: &[CHECKPOINT], - }; + // Same name, different signal: each checkpoint is individually valid, + // so the pair only trips the device-level duplicate check. + const CHECKPOINT_DUP: BootCheckpoint<u8> = + BootCheckpoint::new("boot-complete", 1, Duration::from_secs(1)); #[test] fn accepts_a_valid_table() { - validate(&[DEVICE]); + let device = DeviceConfig::new("dev", 0u8, &[CHECKPOINT]); + assert_eq!(device.name(), "dev"); + assert_eq!(*device.reset_signal(), 0); + assert_eq!(device.checkpoints().len(), 1); + assert_eq!(device.checkpoints()[0].name(), "boot-complete"); + assert_eq!(*device.checkpoints()[0].signal(), 0); + assert_eq!(device.checkpoints()[0].timeout(), Duration::from_secs(1)); } #[test] #[should_panic(expected = "checkpoint names must be unique")] fn rejects_duplicate_checkpoint_names() { - validate(&[DeviceConfig { - checkpoints: &[ - CHECKPOINT, - BootCheckpoint { - signal: 1, - ..CHECKPOINT - }, - ], - ..DEVICE - }]); + let _ = DeviceConfig::new("dev", 0u8, &[CHECKPOINT, CHECKPOINT_DUP]); } #[test] #[should_panic(expected = "device name must not be empty")] fn rejects_an_empty_device_name() { - validate(&[DEVICE, DeviceConfig { name: "", ..DEVICE }]); + let _ = DeviceConfig::new("", 0u8, &[CHECKPOINT]); } #[test] #[should_panic(expected = "at least one boot checkpoint")] fn rejects_an_empty_checkpoint_list() { - validate(&[DeviceConfig { - checkpoints: &[], - ..DEVICE - }]); + let _ = DeviceConfig::new("dev", 0u8, &[] as &[BootCheckpoint<u8>]); } #[test] #[should_panic(expected = "checkpoint name must not be empty")] fn rejects_an_empty_checkpoint_name() { - validate(&[DeviceConfig { - checkpoints: &[BootCheckpoint { - name: "", - ..CHECKPOINT - }], - ..DEVICE - }]); + let _ = BootCheckpoint::new("", 0u8, Duration::from_secs(1)); } #[test] #[should_panic(expected = "checkpoint timeout must not be zero")] fn rejects_a_zero_checkpoint_timeout() { - validate(&[DeviceConfig { - checkpoints: &[BootCheckpoint { - timeout: Duration::ZERO, - ..CHECKPOINT - }], - ..DEVICE - }]); + let _ = BootCheckpoint::new("boot-complete", 0u8, Duration::ZERO); } }
diff --git a/target/mock/devices.rs b/target/mock/devices.rs index 33af731..9880269 100644 --- a/target/mock/devices.rs +++ b/target/mock/devices.rs
@@ -33,45 +33,38 @@ pub const MANAGED_DEVICES: &[DeviceConfig<u8, MockSignal>] = &[ // Direct-flash SPI device (BMC archetype): the eRoT fronts its flash. // Single checkpoint: it raises a boot-complete GPIO. - DeviceConfig { - name: "bmc", - reset_signal: 7, - checkpoints: &[BootCheckpoint { - name: "boot-complete", - signal: MockSignal::Gpio(12), - timeout: Duration::from_secs(90), - }], - }, + DeviceConfig::new( + "bmc", + 7, + &[BootCheckpoint::new( + "boot-complete", + MockSignal::Gpio(12), + Duration::from_secs(90), + )], + ), // PLDM device (NIC archetype): self-updating, SPDM-capable. Two // checkpoints, exercising the multi-checkpoint path: transport up // first, then proof the workload is alive. - DeviceConfig { - name: "nic", - reset_signal: 3, - checkpoints: &[ - BootCheckpoint { - name: "mctp-ready", - signal: MockSignal::MctpReady, - timeout: Duration::from_secs(20), - }, - BootCheckpoint { - name: "heartbeat", - signal: MockSignal::Heartbeat, - timeout: Duration::from_secs(10), - }, + DeviceConfig::new( + "nic", + 3, + &[ + BootCheckpoint::new("mctp-ready", MockSignal::MctpReady, Duration::from_secs(20)), + BootCheckpoint::new("heartbeat", MockSignal::Heartbeat, Duration::from_secs(10)), ], - }, + ), ]; -/// Board-local checks the generic `validate` cannot do — it knows the -/// schema's shape, not this board's meanings. Same const-fence pattern: -/// a bad signal fails the build. +/// Board-local checks the schema constructors cannot do — they know the +/// schema's shape, not this board's meanings. Const-fence pattern: a bad +/// signal fails the build. const fn validate_signals(devices: &[DeviceConfig<u8, MockSignal>]) { let mut i = 0; while i < devices.len() { + let checkpoints = devices[i].checkpoints(); let mut c = 0; - while c < devices[i].checkpoints.len() { - if let MockSignal::Gpio(line) = devices[i].checkpoints[c].signal { + while c < checkpoints.len() { + if let MockSignal::Gpio(line) = *checkpoints[c].signal() { // The mock ready-line bank packs 32 lines, SGPIO-style. assert!(line < 32, "gpio signal names a line outside the bank"); } @@ -81,7 +74,4 @@ } } -const _: () = { - orchestrator_config::validate(MANAGED_DEVICES); - validate_signals(MANAGED_DEVICES); -}; +const _: () = validate_signals(MANAGED_DEVICES);