Skip to content

Commit 4f60515

Browse files
authoredFeb 20, 2025
propolis-cli: check for duplicate spec keys when parsing toml (#865)
otherwise if a TOML defines a block device and disk device the same name, say, calling both "boot", one will clobber the other and the CLI tool will send a malformed request to propolis-server. now you get a more helpful error: ``` Error: spec key Name("boot") defined multiple times ```
1 parent 1dea869 commit 4f60515

File tree

1 file changed

+55
-20
lines changed
  • crates/propolis-config-toml/src

1 file changed

+55
-20
lines changed
 

‎crates/propolis-config-toml/src/spec.rs

+55-20
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,9 @@ pub enum TomlToSpecError {
3737
#[error("failed to parse PCI path string {0:?}")]
3838
PciPathParseFailed(String, #[source] std::io::Error),
3939

40+
#[error("spec key {0:?} defined multiple times")]
41+
DuplicateSpecKey(SpecKey),
42+
4043
#[error("invalid storage device kind {kind:?} for device {name:?}")]
4144
InvalidStorageDeviceType { kind: String, name: String },
4245

@@ -71,6 +74,27 @@ pub struct SpecConfig {
7174
pub components: BTreeMap<SpecKey, ComponentV0>,
7275
}
7376

77+
// Inspired by `api_spec_v0.rs`'s `insert_component` and
78+
// `propolis-cli/src/main.rs`'s `add_component_to_spec`. Same purpose as both of
79+
// them.
80+
//
81+
// Before either of those are transforming one kind of spec to another, TOML
82+
// configurations are parsed to a SpecConfig in this file, where there is *also*
83+
// an opportunity for duplicate keys to clobber spec items.
84+
#[track_caller]
85+
fn spec_component_add(
86+
spec: &mut SpecConfig,
87+
key: SpecKey,
88+
component: ComponentV0,
89+
) -> Result<(), TomlToSpecError> {
90+
if spec.components.contains_key(&key) {
91+
return Err(TomlToSpecError::DuplicateSpecKey(key));
92+
}
93+
94+
spec.components.insert(key, component);
95+
Ok(())
96+
}
97+
7498
impl TryFrom<&super::Config> for SpecConfig {
7599
type Error = TomlToSpecError;
76100

@@ -109,12 +133,13 @@ impl TryFrom<&super::Config> for SpecConfig {
109133
.unwrap_or(0)
110134
.max(0) as u32;
111135

112-
spec.components.insert(
136+
spec_component_add(
137+
&mut spec,
113138
SpecKey::Name(MIGRATION_FAILURE_DEVICE_NAME.to_owned()),
114139
ComponentV0::MigrationFailureInjector(
115140
MigrationFailureInjector { fail_exports, fail_imports },
116141
),
117-
);
142+
)?;
118143

119144
continue;
120145
}
@@ -140,20 +165,24 @@ impl TryFrom<&super::Config> for SpecConfig {
140165
backend_config,
141166
)?;
142167

143-
spec.components.insert(device_id, device_spec);
144-
spec.components.insert(backend_id, backend_spec);
168+
spec_component_add(&mut spec, device_id, device_spec)?;
169+
spec_component_add(&mut spec, backend_id, backend_spec)?;
145170
}
146171
"pci-virtio-viona" => {
147172
let ParsedNic { device_spec, backend_spec, backend_id } =
148173
parse_network_device_from_config(device_name, device)?;
149174

150-
spec.components
151-
.insert(device_id, ComponentV0::VirtioNic(device_spec));
175+
spec_component_add(
176+
&mut spec,
177+
device_id,
178+
ComponentV0::VirtioNic(device_spec),
179+
)?;
152180

153-
spec.components.insert(
181+
spec_component_add(
182+
&mut spec,
154183
backend_id,
155184
ComponentV0::VirtioNetworkBackend(backend_spec),
156-
);
185+
)?;
157186
}
158187
"softnpu-pci-port" => {
159188
let pci_path: PciPath =
@@ -163,12 +192,13 @@ impl TryFrom<&super::Config> for SpecConfig {
163192
)
164193
})?;
165194

166-
spec.components.insert(
195+
spec_component_add(
196+
&mut spec,
167197
device_id,
168198
ComponentV0::SoftNpuPciPort(SoftNpuPciPort {
169199
pci_path,
170200
}),
171-
);
201+
)?;
172202
}
173203
"softnpu-port" => {
174204
let vnic_name =
@@ -179,20 +209,22 @@ impl TryFrom<&super::Config> for SpecConfig {
179209
let backend_name =
180210
SpecKey::Name(format!("{device_id}:backend"));
181211

182-
spec.components.insert(
212+
spec_component_add(
213+
&mut spec,
183214
device_id,
184215
ComponentV0::SoftNpuPort(SoftNpuPort {
185216
link_name: device_name.to_string(),
186217
backend_id: backend_name.clone(),
187218
}),
188-
);
219+
)?;
189220

190-
spec.components.insert(
221+
spec_component_add(
222+
&mut spec,
191223
backend_name,
192224
ComponentV0::DlpiNetworkBackend(DlpiNetworkBackend {
193225
vnic_name: vnic_name.to_owned(),
194226
}),
195-
);
227+
)?;
196228
}
197229
"softnpu-p9" => {
198230
let pci_path: PciPath =
@@ -202,19 +234,21 @@ impl TryFrom<&super::Config> for SpecConfig {
202234
)
203235
})?;
204236

205-
spec.components.insert(
237+
spec_component_add(
238+
&mut spec,
206239
device_id,
207240
ComponentV0::SoftNpuP9(SoftNpuP9 { pci_path }),
208-
);
241+
)?;
209242
}
210243
"pci-virtio-9p" => {
211-
spec.components.insert(
244+
spec_component_add(
245+
&mut spec,
212246
device_id,
213247
ComponentV0::P9fs(parse_p9fs_from_config(
214248
device_name,
215249
device,
216250
)?),
217-
);
251+
)?;
218252
}
219253
_ => {
220254
return Err(TomlToSpecError::UnrecognizedDeviceType(
@@ -233,13 +267,14 @@ impl TryFrom<&super::Config> for SpecConfig {
233267
)
234268
})?;
235269

236-
spec.components.insert(
270+
spec_component_add(
271+
&mut spec,
237272
SpecKey::Name(format!("pci-bridge-{}", bridge.pci_path)),
238273
ComponentV0::PciPciBridge(PciPciBridge {
239274
downstream_bus: bridge.downstream_bus,
240275
pci_path,
241276
}),
242-
);
277+
)?;
243278
}
244279

245280
Ok(spec)

0 commit comments

Comments
 (0)