From 157f0e9e24117a9440a901f39d6d4aeca5ffb834 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 28 Oct 2025 11:11:26 -0400 Subject: [PATCH 1/5] Working on better ignition target selection --- faux-mgs/src/main.rs | 150 +++++++++++++++++++++++++++++++++++++++---- 1 file changed, 136 insertions(+), 14 deletions(-) diff --git a/faux-mgs/src/main.rs b/faux-mgs/src/main.rs index 2894a03..be26331 100644 --- a/faux-mgs/src/main.rs +++ b/faux-mgs/src/main.rs @@ -172,20 +172,23 @@ enum Command { /// Ask SP for its current state. State, - /// Get the ignition state for a single target port (only valid if the SP is - /// an ignition controller). + /// Get the ignition state for a port (only valid if the SP is an ignition + /// controller). Ignition { + #[clap(flatten)] + sel: IgnitionSelector, #[clap( help = "integer of a target, or 'all' for all targets", value_parser = IgnitionLinkEventsTarget::parse, )] - target: IgnitionLinkEventsTarget, + tgt: Option, }, /// Send an ignition command for a single target port (only valid if the SP /// is an ignition controller). IgnitionCommand { - target: u8, + #[clap(flatten)] + sel: IgnitionSelector, #[clap( help = "'power-on', 'power-off', or 'power-reset'", value_parser = ignition_command_from_str, @@ -643,6 +646,108 @@ impl std::fmt::Display for CfpaSlot { } } +#[derive(Clone, Debug, clap::Args)] +pub struct IgnitionSelector { + /// Ignition target + #[clap(short, long)] + target: Option, + + /// Sled cubby (0-31) + #[clap(short, long, conflicts_with_all = ["target"])] + cubby: Option, + + /// Sidecar ('local' or 'peer') + #[clap( + short, long, conflicts_with_all = ["target", "cubby"], + value_parser = parse_sidecar_selector + )] + sidecar: Option, + + /// PSC index (0-1) + #[clap(short, long, conflicts_with_all = ["target", "cubby", "sidecar"])] + psc: Option, +} + +impl IgnitionSelector { + /// Decodes CLI arguments to an ignition target + fn get_target(&self, log: &Logger) -> Result> { + // At this point, we assume that the various `Option` values are + // mutually exclusive (enforced by `clap`). + let t = if let Some(t) = self.target { + t + } else if let Some(c) = self.cubby { + let t = match c { + 0 => 14, + 1 => 30, + 2 => 15, + 3 => 31, + 4 => 13, + 5 => 29, + 6 => 12, + 7 => 28, + 8 => 10, + 9 => 26, + 10 => 11, + 11 => 27, + 12 => 9, + 13 => 25, + 14 => 8, + 15 => 24, + 16 => 4, + 17 => 20, + 18 => 5, + 19 => 21, + 20 => 7, + 22 => 6, + 24 => 0, + 25 => 16, + 26 => 1, + 27 => 17, + 28 => 3, + 29 => 19, + 30 => 2, + 31 => 18, + i => bail!("cubby must be in the range 0-31, not {i}"), + }; + debug!(log, "decoded cubby {c} => target {t}"); + t + } else if let Some(s) = self.sidecar { + let t = match s { + SidecarSelector::Peer => 34, + SidecarSelector::Local => 35, + }; + debug!(log, "decoded {s:?} => target {t}"); + t + } else if let Some(p) = self.psc { + let t = match p { + 0 => 32, + 1 => 33, + i => bail!("psc must be in the range 0-1, not {i}"), + }; + debug!(log, "decoded psc {p} => target {t}"); + t + } else { + return Ok(None); + }; + Ok(Some(t)) + } +} + +#[derive(Copy, Clone, Debug)] +enum SidecarSelector { + Local, + Peer, +} + +fn parse_sidecar_selector(s: &str) -> Result { + let out = match s.to_ascii_lowercase().as_str() { + "local" => SidecarSelector::Local, + "peer" => SidecarSelector::Peer, + _ => bail!("invalid sidecar selector '{s}', must be 'local' or 'peer'"), + }; + Ok(out) +} + impl Command { // If the user didn't specify a listening port, what should we use? We // allow this to vary by command so that client commands (most of them) can @@ -1212,17 +1317,31 @@ async fn run_command( Ok(Output::Lines(lines)) } } - Command::Ignition { target } => { + Command::Ignition { sel, tgt } => { let mut by_target = BTreeMap::new(); - if let Some(target) = target.0 { - let state = sp.ignition_state(target).await?; - by_target.insert(usize::from(target), state); - } else { - let states = sp.bulk_ignition_state().await?; - for (i, state) in states.into_iter().enumerate() { - by_target.insert(i, state); + match (sel.get_target(&log)?, tgt) { + (Some(_), Some(IgnitionLinkEventsTarget(None))) => { + bail!("cannot specify a target and `all`") } - } + (Some(_), Some(IgnitionLinkEventsTarget(Some(..)))) => { + bail!( + "cannot specify target with both flag and \ + standalone argument" + ) + } + (Some(target), None) + | (None, Some(IgnitionLinkEventsTarget(Some(target)))) => { + let state = sp.ignition_state(target).await?; + by_target.insert(usize::from(target), state); + } + (None, Some(IgnitionLinkEventsTarget(None))) => { + let states = sp.bulk_ignition_state().await?; + for (i, state) in states.into_iter().enumerate() { + by_target.insert(i, state); + } + } + (None, None) => (), + }; if json { Ok(Output::Json(serde_json::to_value(by_target).unwrap())) } else { @@ -1233,7 +1352,10 @@ async fn run_command( Ok(Output::Lines(lines)) } } - Command::IgnitionCommand { target, command } => { + Command::IgnitionCommand { sel, command } => { + let target = sel.get_target(&log)?.ok_or_else(|| { + anyhow!("must specify --target, --cubby, --sidecar, or --psc") + })?; sp.ignition_command(target, command).await?; info!(log, "ignition command {command:?} send to target {target}"); if json { From 623aacccec4c49a21a7555242669934620a0f14e Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 28 Oct 2025 11:20:10 -0400 Subject: [PATCH 2/5] Also use selector for other commands --- faux-mgs/src/main.rs | 115 +++++++++++++++++++++---------------------- 1 file changed, 56 insertions(+), 59 deletions(-) diff --git a/faux-mgs/src/main.rs b/faux-mgs/src/main.rs index be26331..6c2bfbc 100644 --- a/faux-mgs/src/main.rs +++ b/faux-mgs/src/main.rs @@ -176,12 +176,7 @@ enum Command { /// controller). Ignition { #[clap(flatten)] - sel: IgnitionSelector, - #[clap( - help = "integer of a target, or 'all' for all targets", - value_parser = IgnitionLinkEventsTarget::parse, - )] - tgt: Option, + sel: IgnitionBulkSelector, }, /// Send an ignition command for a single target port (only valid if the SP @@ -199,21 +194,15 @@ enum Command { /// Get bulk ignition link events (only valid if the SP is an ignition /// controller). IgnitionLinkEvents { - #[clap( - help = "integer of a target, or 'all' for all targets", - value_parser = IgnitionLinkEventsTarget::parse, - )] - target: IgnitionLinkEventsTarget, + #[clap(flatten)] + sel: IgnitionBulkSelector, }, /// Clear all ignition link events (only valid if the SP is an ignition /// controller). ClearIgnitionLinkEvents { - #[clap( - help = "integer of a target, or 'all' for all targets", - value_parser = IgnitionLinkEventsTarget::parse, - )] - target: IgnitionLinkEventsTarget, + #[clap(flatten)] + sel: IgnitionBulkSelector, #[clap( help = "'controller', 'target-link0', 'target-link1', or 'all'", value_parser = IgnitionLinkEventsTransceiverSelect::parse, @@ -676,6 +665,7 @@ impl IgnitionSelector { let t = if let Some(t) = self.target { t } else if let Some(c) = self.cubby { + // See RFD 144 ยง 6.1 (Switch Port Map) for this mapping let t = match c { 0 => 14, 1 => 30, @@ -733,6 +723,35 @@ impl IgnitionSelector { } } +/// `IgnitionSelector` that also supports `--all` +#[derive(Clone, Debug, clap::Args)] +pub struct IgnitionBulkSelector { + #[clap(flatten)] + sel: IgnitionSelector, + /// All targets + #[clap(short, long)] + all: bool, +} + +#[derive(Copy, Clone, Debug)] +enum IgnitionBulkTargets { + Single(u8), + All, +} + +impl IgnitionBulkSelector { + fn get_targets(&self, log: &Logger) -> Result { + match (self.sel.get_target(log)?, self.all) { + (Some(_), true) => { + bail!("cannot specify a target and `--all`") + } + (Some(target), false) => Ok(IgnitionBulkTargets::Single(target)), + (None, true) => Ok(IgnitionBulkTargets::All), + (None, false) => bail!("must specify either a target or `--all`"), + } + } +} + #[derive(Copy, Clone, Debug)] enum SidecarSelector { Local, @@ -779,23 +798,6 @@ fn parse_sp_component(component: &str) -> Result { .map_err(|_| anyhow!("invalid component name: {component}")) } -#[derive(Debug, Clone, Copy)] -struct IgnitionLinkEventsTarget(Option); - -impl IgnitionLinkEventsTarget { - fn parse(s: &str) -> Result { - match s { - "all" | "ALL" => Ok(Self(None)), - _ => { - let target = s - .parse() - .with_context(|| "must be an integer (0..256) or 'all'")?; - Ok(Self(Some(target))) - } - } - } -} - #[derive(Debug, Clone, Copy)] struct IgnitionLinkEventsTransceiverSelect(Option); @@ -1317,30 +1319,19 @@ async fn run_command( Ok(Output::Lines(lines)) } } - Command::Ignition { sel, tgt } => { + Command::Ignition { sel } => { let mut by_target = BTreeMap::new(); - match (sel.get_target(&log)?, tgt) { - (Some(_), Some(IgnitionLinkEventsTarget(None))) => { - bail!("cannot specify a target and `all`") - } - (Some(_), Some(IgnitionLinkEventsTarget(Some(..)))) => { - bail!( - "cannot specify target with both flag and \ - standalone argument" - ) - } - (Some(target), None) - | (None, Some(IgnitionLinkEventsTarget(Some(target)))) => { + match sel.get_targets(&log)? { + IgnitionBulkTargets::Single(target) => { let state = sp.ignition_state(target).await?; by_target.insert(usize::from(target), state); } - (None, Some(IgnitionLinkEventsTarget(None))) => { + IgnitionBulkTargets::All => { let states = sp.bulk_ignition_state().await?; for (i, state) in states.into_iter().enumerate() { by_target.insert(i, state); } } - (None, None) => (), }; if json { Ok(Output::Json(serde_json::to_value(by_target).unwrap())) @@ -1366,15 +1357,18 @@ async fn run_command( )])) } } - Command::IgnitionLinkEvents { target } => { + Command::IgnitionLinkEvents { sel } => { let mut by_target = BTreeMap::new(); - if let Some(target) = target.0 { - let events = sp.ignition_link_events(target).await?; - by_target.insert(usize::from(target), events); - } else { - let events = sp.bulk_ignition_link_events().await?; - for (i, events) in events.into_iter().enumerate() { - by_target.insert(i, events); + match sel.get_targets(&log)? { + IgnitionBulkTargets::Single(target) => { + let events = sp.ignition_link_events(target).await?; + by_target.insert(usize::from(target), events); + } + IgnitionBulkTargets::All => { + let events = sp.bulk_ignition_link_events().await?; + for (i, events) in events.into_iter().enumerate() { + by_target.insert(i, events); + } } } if json { @@ -1387,9 +1381,12 @@ async fn run_command( Ok(Output::Lines(lines)) } } - Command::ClearIgnitionLinkEvents { target, transceiver_select } => { - sp.clear_ignition_link_events(target.0, transceiver_select.0) - .await?; + Command::ClearIgnitionLinkEvents { sel, transceiver_select } => { + let target = match sel.get_targets(&log)? { + IgnitionBulkTargets::Single(t) => Some(t), + IgnitionBulkTargets::All => None, + }; + sp.clear_ignition_link_events(target, transceiver_select.0).await?; info!(log, "ignition link events cleared"); if json { Ok(Output::Json(json!({ "ack": "cleared" }))) From e85b7bc9c1a82f97277a1efd1ffefe9cbcfe384d Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 28 Oct 2025 14:09:09 -0400 Subject: [PATCH 3/5] Backwards compatibility --- faux-mgs/src/main.rs | 77 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 68 insertions(+), 9 deletions(-) diff --git a/faux-mgs/src/main.rs b/faux-mgs/src/main.rs index 6c2bfbc..cee4517 100644 --- a/faux-mgs/src/main.rs +++ b/faux-mgs/src/main.rs @@ -183,7 +183,7 @@ enum Command { /// is an ignition controller). IgnitionCommand { #[clap(flatten)] - sel: IgnitionSelector, + sel: IgnitionSingleSelector, #[clap( help = "'power-on', 'power-off', or 'power-reset'", value_parser = ignition_command_from_str, @@ -657,6 +657,23 @@ pub struct IgnitionSelector { psc: Option, } +#[derive(Clone, Debug, clap::Args)] +#[clap(group(clap::ArgGroup::new("select") + .required(true) + .args(&["target", "cubby", "sidecar", "psc"])) +)] +pub struct IgnitionSingleSelector { + #[clap(flatten)] + sel: IgnitionSelector, +} + +impl IgnitionSingleSelector { + fn get_target(&self, log: &Logger) -> Result { + // presence of at least one is enforced by clap + Ok(self.sel.get_target(log)?.unwrap()) + } +} + impl IgnitionSelector { /// Decodes CLI arguments to an ignition target fn get_target(&self, log: &Logger) -> Result> { @@ -725,12 +742,21 @@ impl IgnitionSelector { /// `IgnitionSelector` that also supports `--all` #[derive(Clone, Debug, clap::Args)] +#[clap(group(clap::ArgGroup::new("select") + .required(true) + .args(&["target", "cubby", "sidecar", "psc", "all", "compat"])) +)] pub struct IgnitionBulkSelector { #[clap(flatten)] sel: IgnitionSelector, + /// All targets #[clap(short, long)] all: bool, + + /// 'all' or a target number (backwards-compatibility shim) + #[clap(value_parser = IgnitionSelectorCompatibilityShim::parse)] + compat: Option, } #[derive(Copy, Clone, Debug)] @@ -741,13 +767,31 @@ enum IgnitionBulkTargets { impl IgnitionBulkSelector { fn get_targets(&self, log: &Logger) -> Result { - match (self.sel.get_target(log)?, self.all) { - (Some(_), true) => { + match (self.sel.get_target(log)?, self.all, self.compat) { + (Some(_), true, _) | (_, true, Some(_)) => { bail!("cannot specify a target and `--all`") } - (Some(target), false) => Ok(IgnitionBulkTargets::Single(target)), - (None, true) => Ok(IgnitionBulkTargets::All), - (None, false) => bail!("must specify either a target or `--all`"), + (Some(_), _, Some(_)) => { + bail!( + "cannot specify a target with both flags \ + and positional arguments" + ) + } + (Some(target), false, None) => { + Ok(IgnitionBulkTargets::Single(target)) + } + (None, false, Some(t)) => { + if let Some(t) = t.0 { + Ok(IgnitionBulkTargets::Single(t)) + } else { + Ok(IgnitionBulkTargets::All) + } + } + (None, true, None) => Ok(IgnitionBulkTargets::All), + (None, false, None) => { + // enforced by clap + panic!("must specify either a target or `--all`") + } } } } @@ -798,6 +842,23 @@ fn parse_sp_component(component: &str) -> Result { .map_err(|_| anyhow!("invalid component name: {component}")) } +#[derive(Debug, Clone, Copy)] +struct IgnitionSelectorCompatibilityShim(Option); + +impl IgnitionSelectorCompatibilityShim { + fn parse(s: &str) -> Result { + match s { + "all" | "ALL" => Ok(Self(None)), + _ => { + let target = s + .parse() + .with_context(|| "must be an integer (0..256) or 'all'")?; + Ok(Self(Some(target))) + } + } + } +} + #[derive(Debug, Clone, Copy)] struct IgnitionLinkEventsTransceiverSelect(Option); @@ -1344,9 +1405,7 @@ async fn run_command( } } Command::IgnitionCommand { sel, command } => { - let target = sel.get_target(&log)?.ok_or_else(|| { - anyhow!("must specify --target, --cubby, --sidecar, or --psc") - })?; + let target = sel.get_target(&log)?; sp.ignition_command(target, command).await?; info!(log, "ignition command {command:?} send to target {target}"); if json { From 735c92061923caebcbea8705f9f12a521d1c11d0 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Tue, 28 Oct 2025 14:44:11 -0400 Subject: [PATCH 4/5] Small cleanup --- faux-mgs/src/main.rs | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/faux-mgs/src/main.rs b/faux-mgs/src/main.rs index cee4517..b631c19 100644 --- a/faux-mgs/src/main.rs +++ b/faux-mgs/src/main.rs @@ -755,8 +755,8 @@ pub struct IgnitionBulkSelector { all: bool, /// 'all' or a target number (backwards-compatibility shim) - #[clap(value_parser = IgnitionSelectorCompatibilityShim::parse)] - compat: Option, + #[clap(value_parser = parse_bulk_targets_shim)] + compat: Option, } #[derive(Copy, Clone, Debug)] @@ -780,13 +780,7 @@ impl IgnitionBulkSelector { (Some(target), false, None) => { Ok(IgnitionBulkTargets::Single(target)) } - (None, false, Some(t)) => { - if let Some(t) = t.0 { - Ok(IgnitionBulkTargets::Single(t)) - } else { - Ok(IgnitionBulkTargets::All) - } - } + (None, false, Some(t)) => Ok(t), (None, true, None) => Ok(IgnitionBulkTargets::All), (None, false, None) => { // enforced by clap @@ -842,19 +836,14 @@ fn parse_sp_component(component: &str) -> Result { .map_err(|_| anyhow!("invalid component name: {component}")) } -#[derive(Debug, Clone, Copy)] -struct IgnitionSelectorCompatibilityShim(Option); - -impl IgnitionSelectorCompatibilityShim { - fn parse(s: &str) -> Result { - match s { - "all" | "ALL" => Ok(Self(None)), - _ => { - let target = s - .parse() - .with_context(|| "must be an integer (0..256) or 'all'")?; - Ok(Self(Some(target))) - } +fn parse_bulk_targets_shim(s: &str) -> Result { + match s { + "all" | "ALL" => Ok(IgnitionBulkTargets::All), + _ => { + let target = s + .parse() + .with_context(|| "must be an integer (0..256) or 'all'")?; + Ok(IgnitionBulkTargets::Single(target)) } } } From dc5e155175775f60836602b7617dfd0d488f3c37 Mon Sep 17 00:00:00 2001 From: Matt Keeter Date: Wed, 5 Nov 2025 12:12:28 -0500 Subject: [PATCH 5/5] Fix compatibility shim --- faux-mgs/src/main.rs | 77 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 64 insertions(+), 13 deletions(-) diff --git a/faux-mgs/src/main.rs b/faux-mgs/src/main.rs index b631c19..9de45fe 100644 --- a/faux-mgs/src/main.rs +++ b/faux-mgs/src/main.rs @@ -176,12 +176,14 @@ enum Command { /// controller). Ignition { #[clap(flatten)] - sel: IgnitionBulkSelector, + sel: IgnitionBulkSelectorWithCompatibilityShim, }, /// Send an ignition command for a single target port (only valid if the SP /// is an ignition controller). IgnitionCommand { + // We can't use the compatibility shim here because it has an optional + // positional argument, which can't be followed by `command` #[clap(flatten)] sel: IgnitionSingleSelector, #[clap( @@ -195,12 +197,14 @@ enum Command { /// controller). IgnitionLinkEvents { #[clap(flatten)] - sel: IgnitionBulkSelector, + sel: IgnitionBulkSelectorWithCompatibilityShim, }, /// Clear all ignition link events (only valid if the SP is an ignition /// controller). ClearIgnitionLinkEvents { + // We can't use the compatibility shim here because it has an optional + // positional argument, which can't be followed by `transceiver_select` #[clap(flatten)] sel: IgnitionBulkSelector, #[clap( @@ -635,7 +639,7 @@ impl std::fmt::Display for CfpaSlot { } } -#[derive(Clone, Debug, clap::Args)] +#[derive(Copy, Clone, Debug, clap::Args)] pub struct IgnitionSelector { /// Ignition target #[clap(short, long)] @@ -744,7 +748,7 @@ impl IgnitionSelector { #[derive(Clone, Debug, clap::Args)] #[clap(group(clap::ArgGroup::new("select") .required(true) - .args(&["target", "cubby", "sidecar", "psc", "all", "compat"])) + .args(&["target", "cubby", "sidecar", "psc", "all"])) )] pub struct IgnitionBulkSelector { #[clap(flatten)] @@ -753,19 +757,45 @@ pub struct IgnitionBulkSelector { /// All targets #[clap(short, long)] all: bool, +} - /// 'all' or a target number (backwards-compatibility shim) - #[clap(value_parser = parse_bulk_targets_shim)] - compat: Option, +impl IgnitionBulkSelector { + fn get_targets(&self, log: &Logger) -> Result { + // Delegate to the fancier representation + IgnitionBulkSelectorWithCompatibilityShim { + sel: self.sel, + all: self.all, + compat: None, + } + .get_targets(log) + } } -#[derive(Copy, Clone, Debug)] -enum IgnitionBulkTargets { - Single(u8), - All, +/// `IgnitionBulkSelector` that also supports a positional argument +/// +/// This is for backwards compatibility. However, it can't be used in +/// combination with later positional arguments, because optional positional +/// arguments are only allowed at the end of an argument list. +#[derive(Clone, Debug, clap::Args)] +#[clap(group(clap::ArgGroup::new("select") + .required(true) + .args(&["target", "cubby", "sidecar", "psc", "all", "compat"])) +)] +pub struct IgnitionBulkSelectorWithCompatibilityShim { + #[clap(flatten)] + sel: IgnitionSelector, + + /// All targets + #[clap(short, long)] + all: bool, + + /// 'all' or a target number + /// (deprecated, present for backwards-compatibility) + #[clap(value_parser = parse_bulk_targets_shim)] + compat: Option, } -impl IgnitionBulkSelector { +impl IgnitionBulkSelectorWithCompatibilityShim { fn get_targets(&self, log: &Logger) -> Result { match (self.sel.get_target(log)?, self.all, self.compat) { (Some(_), true, _) | (_, true, Some(_)) => { @@ -780,7 +810,22 @@ impl IgnitionBulkSelector { (Some(target), false, None) => { Ok(IgnitionBulkTargets::Single(target)) } - (None, false, Some(t)) => Ok(t), + (None, false, Some(t)) => { + match t { + IgnitionBulkTargets::All => warn!( + log, + "positional `all` argument is deprecated, \ + please switch to `--all`" + ), + IgnitionBulkTargets::Single(..) => warn!( + log, + "positional target argument is deprecated, \ + please switch to `--target`, `--cubby`, \ + `--sidecar`, or `--psc`" + ), + } + Ok(t) + } (None, true, None) => Ok(IgnitionBulkTargets::All), (None, false, None) => { // enforced by clap @@ -790,6 +835,12 @@ impl IgnitionBulkSelector { } } +#[derive(Copy, Clone, Debug)] +enum IgnitionBulkTargets { + Single(u8), + All, +} + #[derive(Copy, Clone, Debug)] enum SidecarSelector { Local,