Re: [pve-devel] [PATCH proxmox-ve-rs v4 17/22] config: sdn: fabrics: add fabric config

[Gabriel Goller <g.goller@proxmox.com>]

>> [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