Re: [pve-devel] [PATCH proxmox-ve-rs v4 06/22] frr: add openfabric types

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

On 07.07.2025 13:25, Wolfgang Bumiller wrote:
>On Wed, Jul 02, 2025 at 04:49:57PM +0200, Gabriel Goller wrote:
>> [snip]
>> +/// The OpenFabric properties.
>> +///
>> +/// This struct holds all the OpenFabric interface properties. The most important one here is the
>> +/// fabric_id, which ties the interface to a fabric. When serialized these properties all get
>> +/// prefixed with a space (" ") as they are inside the interface block. They serialize roughly to:
>> +///
>> +/// ```text
>> +/// interface ens20
>> +///  ip router openfabric <fabric_id>
>> +///  ipv6 router openfabric <fabric_id>
>> +///  openfabric hello-interval <value>
>> +///  openfabric hello-multiplier <value>
>> +///  openfabric csnp-interval <value>
>> +///  openfabric passive <value>
>> +/// ```
>> +///
>> +/// The is_ipv4 and is_ipv6 properties decide if we need to add `ip router openfabric`, `ipv6
>> +/// router openfabric`, or both. A interface can only be part of a single fabric.
>
>An*

writing skills of a 4th-grader -_-
thanks!

>> +#[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize, Serialize, PartialOrd, Ord)]
>> +pub struct OpenfabricInterface {
>> +    // Note: an interface can only be a part of a single fabric (so no vec needed here)
>> +    pub fabric_id: OpenfabricRouterName,
>> +    pub passive: Option<bool>,
>
>^ So in  `FrrSerializer` we do all this manually - but for the derived
>`Serialize` implementation, shouldn't we have a
>`#[serde(skip_serializing_if = "Option::is_none")` here?
>
>(and probably also down below...)

Umm actually we don't need serde (and Serialize, Deserialize) in
proxmox-frr at all. My first tests where with a serde serializer, which
didn't really work out and I forgot to remove serde :(

Removed serde and all the Serialize/Deserialize derives.

>> +    pub hello_interval: Option<proxmox_sdn_types::openfabric::HelloInterval>,
>> +    pub csnp_interval: Option<proxmox_sdn_types::openfabric::CsnpInterval>,
>> +    pub hello_multiplier: Option<proxmox_sdn_types::openfabric::HelloMultiplier>,
>> +    pub is_ipv4: bool,
>> +    pub is_ipv6: bool,
>> +}
>> +
>> +impl OpenfabricInterface {
>
>If the struct and its fields are all `pub` - why do we need/want getters?

No reason.
Removed all the setters/getters.

>> +    pub fn fabric_id(&self) -> &OpenfabricRouterName {
>> +        &self.fabric_id
>> +    }
>> +    pub fn passive(&self) -> Option<bool> {
>> +        self.passive
>> +    }
>> +    pub fn hello_interval(&self) -> Option<proxmox_sdn_types::openfabric::HelloInterval> {
>> +        self.hello_interval
>> +    }
>> +    pub fn csnp_interval(&self) -> Option<proxmox_sdn_types::openfabric::CsnpInterval> {
>> +        self.csnp_interval
>> +    }
>> +    pub fn hello_multiplier(&self) -> Option<proxmox_sdn_types::openfabric::HelloMultiplier> {
>> +        self.hello_multiplier
>> +    }
>> +    pub fn set_hello_interval(
>> +        &mut self,
>> +        interval: impl Into<Option<proxmox_sdn_types::openfabric::HelloInterval>>,
>> +    ) {
>> +        self.hello_interval = interval.into();
>> +    }
>> +}
>> +
>> [snip]

Thanks for the review!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel