Re: [PATCH proxmox-ve-rs 5/9] frr: add template serializer and serialize fabrics using templates

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

On 25.02.2026 14:43, Stefan Hanreich wrote:
> comments inline
> 
> On 2/3/26 5:05 PM, Gabriel Goller wrote:
> > Add a new serializer which uses templates in
> > `/etc/proxmox-frr/templates` or from the `proxmox-frr-templates` package
> > in `/usr/share/proxmox-frr/templates` to generate the frr config file.
> > Also update the `build_fabric` function and the tests accordingly.
> > 
> > Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> > ---
> >  proxmox-frr/Cargo.toml                        |   3 +
> >  proxmox-frr/debian/control                    |  10 +
> >  proxmox-frr/src/ser/mod.rs                    | 247 ++++++--------
> >  proxmox-frr/src/ser/openfabric.rs             |  26 +-
> >  proxmox-frr/src/ser/ospf.rs                   |  56 +---
> >  proxmox-frr/src/ser/route_map.rs              | 175 ++++------
> >  proxmox-frr/src/ser/serializer.rs             | 259 ++++-----------
> >  proxmox-sdn-types/src/net.rs                  |   4 +-
> >  proxmox-ve-config/src/common/valid.rs         |   4 +-
> >  proxmox-ve-config/src/sdn/fabric/frr.rs       | 302 ++++++++++--------
> >  .../fabric__openfabric_default_pve.snap       |   2 +-
> >  .../fabric__openfabric_default_pve1.snap      |   2 +-
> >  .../fabric__openfabric_dualstack_pve.snap     |  13 +-
> >  .../fabric__openfabric_ipv6_only_pve.snap     |   4 +-
> >  .../fabric__openfabric_multi_fabric_pve1.snap |   2 +-
> >  .../snapshots/fabric__ospf_default_pve.snap   |   2 +-
> >  .../snapshots/fabric__ospf_default_pve1.snap  |   2 +-
> >  .../fabric__ospf_multi_fabric_pve1.snap       |   2 +-
> >  18 files changed, 468 insertions(+), 647 deletions(-)
> > 
> > [snip]
> > diff --git a/proxmox-frr/src/ser/mod.rs b/proxmox-frr/src/ser/mod.rs
> > index a90397b59a9b..666845ecab74 100644
> > --- a/proxmox-frr/src/ser/mod.rs
> > +++ b/proxmox-frr/src/ser/mod.rs
> > @@ -3,104 +3,17 @@ pub mod ospf;
> >  pub mod route_map;
> >  pub mod serializer;
> >  
> > -use std::collections::{BTreeMap, BTreeSet};
> > -use std::fmt::Display;
> > +use std::collections::BTreeMap;
> > +use std::net::IpAddr;
> >  use std::str::FromStr;
> >  
> > -use crate::ser::route_map::{AccessList, ProtocolRouteMap, RouteMap};
> > +use crate::ser::route_map::{AccessListName, AccessListRule, RouteMapEntry, RouteMapName};
> >  
> > +use bon::Builder;
> 
> A lot of structs here derive Builder, but we don't use any of the
> generated builder functionality atm and just instantiate the structs
> directly in ve-config since all the properties are pub. We should imo
> settle on one approach, preferably now, and then use that consistently.
> 
> We use many collection/map types here, and with bon we'd have to write
> some boilerplate for those [1] and add validation for e.g. duplicate
> route_maps [2]. In the case of the BGP router this could become quite
> tricky imo, since for instance the value of ASN / remote-as / .. depends
> on whether we have a BGP controller / fabric / ...
> 
> Additionally, we're creating an instance of FrrConfig in the Perl part,
> by just building the hash in a way that serializes / deserializes into
> the FrrConfig in Rust. With the builder pattern we'd have to expose the
> builder functionality to the perl side somehow. I think directly
> instantiating/mutating + (de)serializing is the better way here for that
> reason, since we get a partial FrrConfig from Perl already and then just
> incrementally add stuff in Rust. So it'd make sense to remove all
> Builder functionality from here for now to save on dependencies /
> compile-time?
> 
> For future additions, e.g. route-maps, we could utilize the Builder
> pattern, since it is entirely contained to Rust and would make sense
> there - but I'd then introduce it in that patch series.
> 
> [1] https://bon-rs.com/guide/typestate-api/builder-fields#custom-fields
> [2] https://bon-rs.com/guide/patterns/fallible-builders#fallible-builders

Good point -- I'm definitely going through and removing the derive where it
isn't needed.

As discussed offlist, this is not super-urgent as the api of proxmox-frr isn't
really stable or critical or something. Nevertheless we should check how much
the compilation-time overhead is for deriving bon for every struct, as bon is
not really lighweight (uses the type-state builder pattern).

We also need to check if we want to add some validation here or if we don't need
that. If we need some validation, I'd go with bon, because it's a uniform method
of doing this, instead of mixing new() methods and TryInto,TryFrom
implementations, etc.
Quickly went over the build_fabrics code and didn't notice any struct that needs
"different" validations, so e.g. sometimes append, sometimes overwrite (so bon
would still work and we wouldn't need two creation methods).

Don't know if we should clean this up and continue with bon, or rip it out
(which wouldn't be much work because we don't use it that heavy) and maybe add
this later on.

Open for some other arguments or opinions!
 
> > +use proxmox_network_types::ip_address::Cidr;
> > +use serde::{Deserialize, Serialize};
> >  use thiserror::Error;
> >  
> > -/// Generic FRR router.
> > -///
> > -/// This generic FRR router contains all the protocols that we implement.
> > -/// In FRR this is e.g.:
> > -/// ```text
> > -/// router openfabric test
> > -/// !....
> > -/// ! or
> > -/// router ospf
> > -/// !....
> > -/// ```
> > -#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
> > -pub enum Router {
> > -    Openfabric(openfabric::OpenfabricRouter),
> > -    Ospf(ospf::OspfRouter),
> > -}
> > -
> > -impl From<openfabric::OpenfabricRouter> for Router {
> > -    fn from(value: openfabric::OpenfabricRouter) -> Self {
> > -        Router::Openfabric(value)
> > -    }
> > -}
> > -
> > -/// Generic FRR routername.
> > -///
> > -/// The variants represent different protocols. Some have `router <protocol> <name>`, others have
> > -/// `router <protocol> <process-id>`, some only have `router <protocol>`.
> > -#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
> > -pub enum RouterName {
> > -    Openfabric(openfabric::OpenfabricRouterName),
> > -    Ospf(ospf::OspfRouterName),
> > -}
> > -
> > -impl From<openfabric::OpenfabricRouterName> for RouterName {
> > -    fn from(value: openfabric::OpenfabricRouterName) -> Self {
> > -        Self::Openfabric(value)
> > -    }
> > -}
> > -
> > -impl Display for RouterName {
> > -    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> > -        match self {
> > -            Self::Openfabric(r) => r.fmt(f),
> > -            Self::Ospf(r) => r.fmt(f),
> > -        }
> > -    }
> > -}
> > -
> > -/// The interface name is the same on ospf and openfabric, but it is an enum so that we can have
> > -/// two different entries in the btreemap. This allows us to have an interface in a ospf and
> > -/// openfabric fabric.
> > -#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
> > -pub enum InterfaceName {
> > -    Openfabric(CommonInterfaceName),
> > -    Ospf(CommonInterfaceName),
> > -}
> > -
> > -impl Display for InterfaceName {
> > -    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> > -        match self {
> > -            InterfaceName::Openfabric(frr_word) => frr_word.fmt(f),
> > -            InterfaceName::Ospf(frr_word) => frr_word.fmt(f),
> > -        }
> > -    }
> > -}
> > -
> > -/// Generic FRR Interface.
> > -///
> > -/// In FRR config it looks like this:
> > -/// ```text
> > -/// interface <name>
> > -/// ! ...
> > -#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
> > -pub enum Interface {
> > -    Openfabric(openfabric::OpenfabricInterface),
> > -    Ospf(ospf::OspfInterface),
> > -}
> > -
> > -impl From<openfabric::OpenfabricInterface> for Interface {
> > -    fn from(value: openfabric::OpenfabricInterface) -> Self {
> > -        Self::Openfabric(value)
> > -    }
> > -}
> > -
> > -impl From<ospf::OspfInterface> for Interface {
> > -    fn from(value: ospf::OspfInterface) -> Self {
> > -        Self::Ospf(value)
> > -    }
> > -}
> > -
> >  #[derive(Error, Debug)]
> >  pub enum FrrWordError {
> >      #[error("word is empty")]
> > @@ -113,7 +26,7 @@ pub enum FrrWordError {
> >  ///
> >  /// Every string argument or value in FRR is an FrrWord. FrrWords must only contain ascii
> >  /// characters and must not have a whitespace.
> > -#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
> > +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
> >  pub struct FrrWord(String);
> >  
> >  impl FrrWord {
> > @@ -144,12 +57,6 @@ impl FromStr for FrrWord {
> >      }
> >  }
> >  
> > -impl Display for FrrWord {
> > -    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> > -        self.0.fmt(f)
> > -    }
> > -}
> > -
> >  impl AsRef<str> for FrrWord {
> >      fn as_ref(&self) -> &str {
> >          &self.0
> > @@ -157,7 +64,7 @@ impl AsRef<str> for FrrWord {
> >  }
> >  
> >  #[derive(Error, Debug)]
> > -pub enum CommonInterfaceNameError {
> > +pub enum InterfaceNameError {
> >      #[error("interface name too long")]
> >      TooLong,
> >  }
> > @@ -166,76 +73,128 @@ pub enum CommonInterfaceNameError {
> >  ///
> >  /// FRR itself doesn't enforce any limits, but the kernel does. Linux only allows interface names
> >  /// to be a maximum of 16 bytes. This is enforced by this struct.
> > -#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
> > -pub struct CommonInterfaceName(String);
> > +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
> > +pub struct InterfaceName(String);
> >  
> > -impl TryFrom<&str> for CommonInterfaceName {
> > -    type Error = CommonInterfaceNameError;
> > +impl TryFrom<&str> for InterfaceName {
> > +    type Error = InterfaceNameError;
> >  
> >      fn try_from(value: &str) -> Result<Self, Self::Error> {
> >          Self::new(value)
> >      }
> >  }
> >  
> > -impl TryFrom<String> for CommonInterfaceName {
> > -    type Error = CommonInterfaceNameError;
> > +impl TryFrom<String> for InterfaceName {
> > +    type Error = InterfaceNameError;
> >  
> >      fn try_from(value: String) -> Result<Self, Self::Error> {
> >          Self::new(value)
> >      }
> >  }
> >  
> > -impl CommonInterfaceName {
> > -    pub fn new<T: AsRef<str> + Into<String>>(s: T) -> Result<Self, CommonInterfaceNameError> {
> > +impl InterfaceName {
> > +    pub fn new<T: AsRef<str> + Into<String>>(s: T) -> Result<Self, InterfaceNameError> {
> 
> Is Into<String> as bound necessary here? Anything we can access as str
> reference can be converted into a string anyway?

Umm I think so because we need a owned String? This way we don't need to force a
clone.

> >          if s.as_ref().len() <= 15 {
> >              Ok(Self(s.into()))
> >          } else {
> > -            Err(CommonInterfaceNameError::TooLong)
> > +            Err(InterfaceNameError::TooLong)
> >          }
> >      }
> >  }
> >  
> > -impl Display for CommonInterfaceName {
> > -    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> > -        self.0.fmt(f)
> > +#[derive(Debug, Clone, Serialize, PartialEq, Eq, Deserialize)]
> > +pub struct Interface<T> {
> > +    #[serde(default, skip_serializing_if = "Vec::is_empty")]
> > +    pub addresses: Vec<Cidr>,
> > +
> > +    #[serde(flatten)]
> > +    pub properties: T,
> > +}
> > +impl From<openfabric::OpenfabricInterface> for Interface<openfabric::OpenfabricInterface> {
> > +    fn from(value: openfabric::OpenfabricInterface) -> Self {
> > +        Interface {
> > +            addresses: Vec::new(),
> > +            properties: value,
> > +        }
> >      }
> >  }
> >  
> > -/// Main FRR config.
> > -///
> > -/// Contains the two main frr building blocks: routers and interfaces. It also holds other
> > -/// top-level FRR options, such as access-lists, router-maps and protocol-routemaps. This struct
> > -/// gets generated using the `FrrConfigBuilder` in `proxmox-ve-config`.
> > -#[derive(Clone, Debug, PartialEq, Eq, Default)]
> > -pub struct FrrConfig {
> > -    pub router: BTreeMap<RouterName, Router>,
> > -    pub interfaces: BTreeMap<InterfaceName, Interface>,
> > -    pub access_lists: Vec<AccessList>,
> > -    pub routemaps: Vec<RouteMap>,
> > -    pub protocol_routemaps: BTreeSet<ProtocolRouteMap>,
> > +impl From<ospf::OspfInterface> for Interface<ospf::OspfInterface> {
> > +    fn from(value: ospf::OspfInterface) -> Self {
> > +        Interface {
> > +            addresses: Vec::new(),
> > +            properties: value,
> > +        }
> > +    }
> >  }
> >  
> > -impl FrrConfig {
> > -    pub fn new() -> Self {
> > -        Self::default()
> > -    }
> > +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
> > +#[serde(untagged)]
> > +pub enum IpOrInterface {
> > +    Ip(IpAddr),
> > +    Interface(InterfaceName),
> > +}
> >  
> > -    pub fn router(&self) -> impl Iterator<Item = (&RouterName, &Router)> + '_ {
> > -        self.router.iter()
> > -    }
> > +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Builder, Deserialize)]
> > +pub struct IpRoute {
> > +    #[serde(default, deserialize_with = "proxmox_serde::perl::deserialize_bool")]
> > +    is_ipv6: bool,
> > +    prefix: Cidr,
> > +    via: IpOrInterface,
> > +    vrf: Option<InterfaceName>,
> > +}
> >  
> > -    pub fn interfaces(&self) -> impl Iterator<Item = (&InterfaceName, &Interface)> + '_ {
> > -        self.interfaces.iter()
> > -    }
> > +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
> > +#[serde(rename_all = "lowercase")]
> > +pub enum FrrProtocol {
> > +    Ospf,
> > +    Openfabric,
> > +    Bgp,
> > +}
> >  
> > -    pub fn access_lists(&self) -> impl Iterator<Item = &AccessList> + '_ {
> > -        self.access_lists.iter()
> > -    }
> > -    pub fn routemaps(&self) -> impl Iterator<Item = &RouteMap> + '_ {
> > -        self.routemaps.iter()
> > -    }
> > +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
> > +pub struct IpProtocolRouteMap {
> > +    pub v4: Option<RouteMapName>,
> > +    pub v6: Option<RouteMapName>,
> > +}
> >  
> > -    pub fn protocol_routemaps(&self) -> impl Iterator<Item = &ProtocolRouteMap> + '_ {
> > -        self.protocol_routemaps.iter()
> > -    }
> > +/// Main FRR config.
> > +///
> > +/// Contains the two main frr building blocks: routers and interfaces. It also holds other
> > +/// top-level FRR options, such as access-lists, router-maps and protocol-routemaps. This struct
> > +/// gets generated using the `FrrConfigBuilder` in `proxmox-ve-config`.
> > +#[derive(Clone, Debug, PartialEq, Eq, Default, Builder, Serialize, Deserialize)]
> > +pub struct FrrConfig {
> > +    #[builder(default)]
> > +    #[serde(default)]
> > +    pub openfabric: OpenfabricFrrConfig,
> > +    #[builder(default)]
> > +    #[serde(default)]
> > +    pub ospf: OspfFrrConfig,
> > +    #[builder(default)]
> > +    #[serde(default)]
> > +    pub protocol_routemaps: BTreeMap<FrrProtocol, IpProtocolRouteMap>,
> > +
> > +    #[builder(default)]
> > +    #[serde(default)]
> > +    pub routemaps: BTreeMap<RouteMapName, Vec<RouteMapEntry>>,
> > +    #[builder(default)]
> > +    #[serde(default)]
> > +    pub access_lists: BTreeMap<AccessListName, Vec<AccessListRule>>,
> > +}
> > +
> > +#[derive(Clone, Debug, PartialEq, Eq, Default, Serialize, Deserialize)]
> > +pub struct OpenfabricFrrConfig {
> > +    #[serde(default)]
> > +    pub router: BTreeMap<openfabric::OpenfabricRouterName, openfabric::OpenfabricRouter>,
> > +    #[serde(default)]
> > +    pub interfaces: BTreeMap<InterfaceName, Interface<openfabric::OpenfabricInterface>>,
> > +}
> > +
> > +#[derive(Clone, Debug, PartialEq, Eq, Default, Serialize, Deserialize)]
> > +pub struct OspfFrrConfig {
> > +    #[serde(default)]
> > +    pub router: Option<ospf::OspfRouter>,
> > +    #[serde(default)]
> > +    pub interfaces: BTreeMap<InterfaceName, Interface<ospf::OspfInterface>>,
> >  }
> > diff --git a/proxmox-frr/src/ser/openfabric.rs b/proxmox-frr/src/ser/openfabric.rs
> > index 0f0c65062d36..8c82c60b0de5 100644
> > --- a/proxmox-frr/src/ser/openfabric.rs
> > +++ b/proxmox-frr/src/ser/openfabric.rs
> > @@ -1,15 +1,17 @@
> >  use std::fmt::Debug;
> > -use std::fmt::Display;
> >  
> > +use bon::Builder;
> >  use proxmox_sdn_types::net::Net;
> >  
> > +use serde::Deserialize;
> > +use serde::Serialize;
> >  use thiserror::Error;
> >  
> >  use crate::ser::FrrWord;
> >  use crate::ser::FrrWordError;
> >  
> >  /// The name of a OpenFabric router. Is an FrrWord.
> > -#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
> > +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
> >  pub struct OpenfabricRouterName(FrrWord);
> >  
> >  impl From<FrrWord> for OpenfabricRouterName {
> > @@ -24,16 +26,16 @@ impl OpenfabricRouterName {
> >      }
> >  }
> >  
> > -impl Display for OpenfabricRouterName {
> > -    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> > -        write!(f, "openfabric {}", self.0)
> > +impl std::fmt::Display for OpenfabricRouterName {
> > +    fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
> > +        self.0.fmt(f)
> >      }
> >  }
> >  
> >  /// All the properties a OpenFabric router can hold.
> >  ///
> >  /// These can serialized with a " " space prefix as they are in the `router openfabric` block.
> > -#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
> > +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
> >  pub struct OpenfabricRouter {
> >      /// The NET address
> >      pub net: Net,
> > @@ -67,15 +69,25 @@ impl OpenfabricRouter {
> >  ///
> >  /// The is_ipv4 and is_ipv6 properties decide if we need to add `ip router openfabric`, `ipv6
> >  /// router openfabric`, or both. An interface can only be part of a single fabric.
> > -#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
> > +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize, Builder)]
> >  pub struct OpenfabricInterface {
> >      // Note: an interface can only be a part of a single fabric (so no vec needed here)
> >      pub fabric_id: OpenfabricRouterName,
> > +    #[serde(
> > +        default,
> > +        deserialize_with = "proxmox_serde::perl::deserialize_bool",
> > +        skip_serializing_if = "Option::is_none"
> > +    )]
> >      pub passive: Option<bool>,
> > +    #[serde(default, skip_serializing_if = "Option::is_none")]
> >      pub hello_interval: Option<proxmox_sdn_types::openfabric::HelloInterval>,
> > +    #[serde(default, skip_serializing_if = "Option::is_none")]
> >      pub csnp_interval: Option<proxmox_sdn_types::openfabric::CsnpInterval>,
> > +    #[serde(default, skip_serializing_if = "Option::is_none")]
> >      pub hello_multiplier: Option<proxmox_sdn_types::openfabric::HelloMultiplier>,
> > +    #[serde(deserialize_with = "proxmox_serde::perl::deserialize_bool")]
> >      pub is_ipv4: bool,
> > +    #[serde(deserialize_with = "proxmox_serde::perl::deserialize_bool")]
> >      pub is_ipv6: bool,
> >  }
> >  
> > diff --git a/proxmox-frr/src/ser/ospf.rs b/proxmox-frr/src/ser/ospf.rs
> > index 67e39a45b8de..8b26f42e2e46 100644
> > --- a/proxmox-frr/src/ser/ospf.rs
> > +++ b/proxmox-frr/src/ser/ospf.rs
> > @@ -1,32 +1,12 @@
> >  use std::fmt::Debug;
> > -use std::fmt::Display;
> >  use std::net::Ipv4Addr;
> >  
> > +use bon::Builder;
> > +use serde::{Deserialize, Serialize};
> >  use thiserror::Error;
> >  
> >  use crate::ser::{FrrWord, FrrWordError};
> >  
> > -/// The name of the ospf frr router.
> > -///
> > -/// We can only have a single ospf router (ignoring multiple invocations of the ospfd daemon)
> > -/// because the router-id needs to be the same between different routers on a single node.
> > -/// We can still have multiple fabrics by separating them using areas. Still, different areas have
> > -/// the same frr router, so the name of the router is just "ospf" in "router ospf".
> > -///
> > -/// This serializes roughly to:
> > -/// ```text
> > -/// router ospf
> > -/// !...
> > -/// ```
> > -#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
> > -pub struct OspfRouterName;
> > -
> > -impl Display for OspfRouterName {
> > -    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> > -        write!(f, "ospf")
> > -    }
> > -}
> > -
> >  #[derive(Error, Debug)]
> >  pub enum AreaParsingError {
> >      #[error("Invalid area idenitifier. Area must be a number or an ipv4 address.")]
> > @@ -44,7 +24,7 @@ pub enum AreaParsingError {
> >  /// or "0" as an area, which then gets translated to "0.0.0.5" and "0.0.0.0" by FRR. We allow both
> >  /// a number or an ip-address. Note that the area "0" (or "0.0.0.0") is a special area - it creates
> >  /// a OSPF "backbone" area.
> > -#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
> > +#[derive(Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord, Serialize, Deserialize)]
> >  pub struct Area(FrrWord);
> >  
> >  impl TryFrom<FrrWord> for Area {
> > @@ -65,12 +45,6 @@ impl Area {
> >      }
> >  }
> >  
> > -impl Display for Area {
> > -    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> > -        write!(f, "area {}", self.0)
> > -    }
> > -}
> > -
> >  /// The OSPF router properties.
> >  ///
> >  /// Currently the only property of a OSPF router is the router_id. The router_id is used to
> > @@ -84,7 +58,7 @@ impl Display for Area {
> >  /// router ospf
> >  ///  router-id <ipv4-address>
> >  /// ```
> 
> Those comments about serialization are not applicable anymore with all
> structs, so it'd make sense to remove them. I've noted where they show
> up in the diff but there are more of them throughout the source code.

Fixed all the serialization comments, thanks for the review!