Re: [PATCH proxmox-ve-rs v4 1/7] sdn: fabric: add BGP protocol support

[Hannes Laimer <h.laimer@proxmox.com>]

thanks, will address in v5!

On 2026-05-13 14:30, Stefan Hanreich wrote:
> On 5/12/26 4:12 PM, Hannes Laimer wrote:
>> +impl AddressFamilies {
>> +    /// Merge another [`AddressFamilies`] into this one.
>> +    ///
>> +    /// For each address family: if `self` already has it, extend its neighbors, networks, and
>> +    /// redistribute lists. If `self` doesn't have it, take it from `other`.
>> +    pub fn merge(&mut self, other: AddressFamilies) {
> 
> maybe extend is the better name, as it mirrors existing conventions from
> std::Vec?
> 

makes sense, can do

[..]

>>              }
>>              FabricEntry::WireGuard(_) => {} // not a frr fabric
>> +            FabricEntry::Bgp(bgp_entry) => {
>> +                let Ok(node) = bgp_entry.node_section(&current_node) else {
>> +                    continue;
>> +                };
>> +
>> +                let BgpNode::Internal(properties) = node.properties() else {
>> +                    continue;
>> +                };
>> +
>> +                let fabric = bgp_entry.fabric_section();
>> +
>> +                let local_asn = properties.asn().as_u32();
> 
> makes me wonder if implementing AsRef<u32> would simplify handling ASN
> throughout the code?
> 

good point, probably also `Deref<Target=u32>`

[..]

>> +
>> +#[api(
>> +    type: Integer,
>> +    minimum: u32::MIN as i64,
>> +    maximum: u32::MAX as i64,
>> +)]
>> +#[derive(Debug, Clone, Serialize, Updater, Hash)]
>> +/// Autonomous system number as defined by RFC 6793
>> +pub struct ASN(u32);
> 
> potentially something for later, but I think we use ASN in several
> places - so we might want to add this to sdn-types instead and reuse it
> across the ve-rs crates?
> 

yes, sounds like a good follow-up that covers all places then

[..]

>> +impl<'de> Deserialize<'de> for ASN {
>> +    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
>> +    where
>> +        D: serde::Deserializer<'de>,
>> +    {
>> +        use serde::de::{self, Visitor};
>> +
>> +        struct AsnVisitor;
>> +
>> +        impl<'de> Visitor<'de> for AsnVisitor {
>> +            type Value = ASN;
>> +
>> +            fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
>> +                formatter.write_str("a u32 or string containing a u32")
>> +            }
>> +
>> +            fn visit_i64<E: de::Error>(self, v: i64) -> Result<ASN, E> {
>> +                u32::try_from(v)
>> +                    .map(ASN)
>> +                    .map_err(|_| E::custom(format!("ASN out of range: {v}")))
>> +            }
>> +
>> +            fn visit_u64<E: de::Error>(self, v: u64) -> Result<ASN, E> {
>> +                u32::try_from(v)
>> +                    .map(ASN)
>> +                    .map_err(|_| E::custom(format!("ASN out of range: {v}")))
>> +            }
>> +
>> +            fn visit_str<E: de::Error>(self, v: &str) -> Result<ASN, E> {
>> +                v.parse::<u32>()
>> +                    .map(ASN)
>> +                    .map_err(|_| E::custom(format!("invalid ASN: {v}")))
>> +            }
>> +        }
>> +
>> +        deserializer.deserialize_any(AsnVisitor)
>> +    }
>> +}
> 
> Is there a reason why proxmox_serde::perl::deserialize_u32 doesn't work?
> It should work the same afaict.
> 

i remeber there was a problem with the information that this is an int
being lost when passed through perlmod, but i tried to reproduce this
again and could not. so no reason :)

[..]