[snip]
diff --git a/proxmox-ve-config/src/sdn/fabric/mod.rs
b/proxmox-ve-config/src/sdn/fabric/mod.rs
index 007be6a3fd8e..3342a7053d3f 100644
--- a/proxmox-ve-config/src/sdn/fabric/mod.rs
+++ b/proxmox-ve-config/src/sdn/fabric/mod.rs
@@ -1 +1,517 @@
pub mod section_config;
+
+use std::collections::BTreeMap;
+use std::marker::PhantomData;
+use std::ops::Deref;
+
+use serde::{Deserialize, Serialize};
+
+use crate::sdn::fabric::section_config::{
+ fabric::{
+ Fabric, FabricDeletableProperties, FabricId, FabricSection,
FabricSectionUpdater,
+ FabricUpdater,
+ },
+ node::{
+ api::{NodeDataUpdater, NodeDeletableProperties, NodeUpdater},
+ Node, NodeId, NodeSection,
+ },
+ protocol::{
+ openfabric::{
+ OpenfabricDeletableProperties, OpenfabricNodeDeletableProperties,
+ OpenfabricNodeProperties, OpenfabricNodePropertiesUpdater,
OpenfabricProperties,
+ OpenfabricPropertiesUpdater,
+ },
+ ospf::{
+ OspfDeletableProperties, OspfNodeDeletableProperties,
OspfNodeProperties,
+ OspfNodePropertiesUpdater, OspfProperties, OspfPropertiesUpdater,
+ },
+ },
+};
+
+#[derive(thiserror::Error, Debug)]
+pub enum FabricConfigError {
+ #[error("fabric '{0}' does not exist in configuration")]
+ FabricDoesNotExist(String),
+ #[error("node '{0}' does not exist in fabric '{1}'")]
+ NodeDoesNotExist(String, String),
^ So, in the "usual" cases, `.get()` methods return an `Option`. Where
this is not the case, we often have an empty error.
Eg.: `io::ErrorKind::NotFound` has no content, `std::num::ParseIntError`
does not include the number/digits.
The names are usually added via `.context()` or some such.
On the other hand, sometimes there are "functional" errors, like
`NulError` in `CString`, since it takes an `Into<Vec<u8>>` it includes
the allocated vector in case the user wants to reuse it in the error
case. Similarly, `Mutex::lock()`'s error is a
`PoisonError<MutexGuard<T>>` which can be turned into the guard to still
access the data...
The use of this below may as well just return an `Option`, but I get
that we want more context, pretty much all cases. I'm just worried we
might end up with more inconsistencies across our various code bases
(callers adding context because they did not expect it to already be
there), so we should probably declare some rules for how to design error
types in our coding style at some point... (This is long overdue!)
Anyway... until we actually define how we want to deal with error types,
we can leave it like this for now...
Yeah I know this is kind of weird. My intention was here to 'force' the
user to add some context to the error. The problem IMO with `.context`
is that sometimes it's forgotten and then you just get e.g. `fabric doesn't
exist`.
+ #[error("node has a different protocol than the referenced fabric")]
+ ProtocolMismatch,
+ #[error("fabric '{0}' already exists in config")]
+ DuplicateFabric(String),
+ #[error("node '{0}' already exists in config for fabric {1}")]
+ DuplicateNode(String, String),
+ // should usually not occur, but we still check for it nonetheless
+ #[error("mismatched fabric_id")]
+ FabricIdMismatch,
+}
+
+/// An entry in a [`FabricConfig`].
+///
+/// It enforces compatible types for its containing [`FabricSection`] and
[`NodeSection`] via the
+/// generic parameters, so only Nodes and Fabrics with compatible types can be
inserted into an
+/// entry.
+#[derive(Debug, Clone, Serialize, Deserialize, Hash)]
+pub struct Entry<F, N> {
+ // we want to store the enum structs Fabric & Node here, in order to have
access to the
+ // properties and methods defined on the enum itself.
+ // In order to still be able to type-check that an Entry contains the
right combination of
+ // NodeSection and FabricSection, we type hint the actual types wrapped into
Fabric & Node here
+ // via PhantomData and only allow insertion of the proper types via the
provided methods.
+ #[serde(skip)]
+ _phantom_fabric: PhantomData<FabricSection<F>>,
+ #[serde(skip)]
+ _phantom_node: PhantomData<NodeSection<N>>,
Technically, since this struct *contains* neither N nor F, this could
use `PhantomData<fn() -> FooSection<X>>`. Here this probably won't make
a difference. In general, though, the way you use the generic type of
`PhantomData` influences auto-traits (Send/Sync), variance and
dropchecking.
Since this struct's Send/Sync don't depend on neither N nor F, the
`fn() -> …` version would be suitable.
See https://doc.rust-lang.org/nomicon/phantom-data.html
Ooh, good point!
Variance stays the same and dropchecking is irrelevant afaict because we don't
store the F and N.
+
+ fabric: Fabric,
+ nodes: BTreeMap<NodeId, Node>,
+}
+
+impl<F, N> Entry<F, N>
+where
+ Fabric: From<FabricSection<F>>,
+ Node: From<NodeSection<N>>,
+{
+ /// Create a new [`Entry`] from the passed [`FabricSection<F>`] with no
nodes.
+ fn new(fabric: FabricSection<F>) -> Self {
+ Self {
+ fabric: fabric.into(),
+ nodes: Default::default(),
+ _phantom_fabric: Default::default(),
+ _phantom_node: Default::default(),
^ `PhantomData` is shorter, const and more obvious than `Default::default()`.
Agree.
+ }
+ }
+
+ /// Adds a node to this entry
+ ///
+ /// # Errors
Please include a newline here.
Done.
+ /// Returns an error if the node's fabric_id doesn't match this entry's
fabric_id
+ /// or if a node with the same ID already exists in this entry.
+ fn add_node(&mut self, node: NodeSection<N>) -> Result<(),
FabricConfigError> {
+ if self.nodes.contains_key(node.id().node_id()) {
+ return Err(FabricConfigError::DuplicateNode(
+ node.id().node_id().to_string(),
+ self.fabric.id().to_string(),
+ ));
+ }
+
+ if node.id().fabric_id() != self.fabric.id() {
+ return Err(FabricConfigError::FabricIdMismatch);
+ }
+
+ self.nodes.insert(node.id().node_id().clone(), node.into());
+
+ Ok(())
+ }
+
[snip]
Thanks for the review!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
