Make address families open values; keep "supported" a separate list

AFI and SAFI were closed enums, so a family without a variant could not be
represented at all, and the set of negotiated families was a struct with
one bool per family. Supporting another family meant touching both.

- `Afi`, `Safi` and `AfiSafi` in bgp-primitives are newtypes over the wire
  numbers, with constants and names for the common families. Capabilities
  and MP_REACH/MP_UNREACH use them, so any family a peer names is read as
  what it is. The special case that turned a capability for an unlisted
  family into an unknown capability is gone.
- `FamilySet` is a set of `AfiSafi`. What we can exchange routes for is the
  explicit `family::SUPPORTED` list; only those are offered in our OPEN.
- `AddressFamilyId` stays as the IP version of a prefix.
- NLRI is still a list of IP prefixes. MP_REACH/MP_UNREACH for a family
  whose NLRI is something else (labelled, VPN, EVPN, ...) is refused
  rather than misread as prefixes, which is what happened before for the
  SAFIs the old enum listed.

Bug found on the way: the reserved octet in MP_REACH_NLRI was skipped only
for IPv6, so an IPv4 MP_REACH_NLRI was parsed with an extra 0.0.0.0/0
route that the peer never sent.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

crates/integration-tests/src/test_peer.rs

@@ -2,7 +2,7 @@ use std::net::{Ipv4Addr, SocketAddr};
 
 use bgp_packet::message::{BGP_HEADER_LEN, Message, UpdateMessage};
 use bgp_packet::open::{Capability, OpenMessage, OpenOption};
-use bgp_primitives::constants::{AddressFamilyId, SubsequentAfi};
+use bgp_primitives::constants::{Afi, Safi};
 use bgp_primitives::parser::ParserContext;
 use bytes::BytesMut;
 use eyre::{Result, bail, eyre};
@@ -104,12 +104,12 @@ impl TestPeer {
         let capabilities = vec![
             Capability::FourByteAsn { asn: local_asn },
             Capability::MultiProtocol {
-                afi: AddressFamilyId::Ipv4,
+                afi: Afi::IPV4,
-                safi: SubsequentAfi::Unicast,
+                safi: Safi::UNICAST,
             },
             Capability::MultiProtocol {
-                afi: AddressFamilyId::Ipv6,
+                afi: Afi::IPV6,
-                safi: SubsequentAfi::Unicast,
+                safi: Safi::UNICAST,
             },
             Capability::RouteRefresh,
         ];

crates/integration-tests/tests/basic_connection.rs

@@ -8,7 +8,7 @@ use bgp_packet::message::{
     Message, MpReachNlriPathAttribute, MpUnreachNlriPathAttribute, NlriNextHop,
     NotificationMessage, PathAttribute, UpdateMessage,
 };
-use bgp_primitives::constants::{AddressFamilyId, SubsequentAfi};
+use bgp_primitives::constants::{AddressFamilyId, Afi, Safi};
 use bgp_primitives::ip_prefix::IpPrefix;
 use bgp_rib::RibEvent;
 use eyre::Result;
@@ -346,8 +346,8 @@ async fn test_server_accepts_ipv6_update() -> Result<()> {
     let update = Message::Update(UpdateMessage {
         withdrawn_routes: vec![],
         path_attributes: vec![PathAttribute::MpReachNlri(MpReachNlriPathAttribute {
-            afi: AddressFamilyId::Ipv6,
+            afi: Afi::IPV6,
-            safi: SubsequentAfi::Unicast,
+            safi: Safi::UNICAST,
             next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()),
             prefixes: vec![prefix],
         })],
@@ -453,8 +453,8 @@ async fn test_rib_records_ipv6_announce() -> Result<()> {
     peer.send(&Message::Update(UpdateMessage {
         withdrawn_routes: vec![],
         path_attributes: vec![PathAttribute::MpReachNlri(MpReachNlriPathAttribute {
-            afi: AddressFamilyId::Ipv6,
+            afi: Afi::IPV6,
-            safi: SubsequentAfi::Unicast,
+            safi: Safi::UNICAST,
             next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()),
             prefixes: vec![prefix.clone()],
         })],
@@ -492,8 +492,8 @@ async fn test_rib_records_ipv6_withdraw() -> Result<()> {
     peer.send(&Message::Update(UpdateMessage {
         withdrawn_routes: vec![],
         path_attributes: vec![PathAttribute::MpReachNlri(MpReachNlriPathAttribute {
-            afi: AddressFamilyId::Ipv6,
+            afi: Afi::IPV6,
-            safi: SubsequentAfi::Unicast,
+            safi: Safi::UNICAST,
             next_hop: NlriNextHop::Ipv6("2001:db8::1".parse().unwrap()),
             prefixes: vec![prefix.clone()],
         })],
@@ -506,8 +506,8 @@ async fn test_rib_records_ipv6_withdraw() -> Result<()> {
     peer.send(&Message::Update(UpdateMessage {
         withdrawn_routes: vec![],
         path_attributes: vec![PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute {
-            afi: AddressFamilyId::Ipv6,
+            afi: Afi::IPV6,
-            safi: SubsequentAfi::Unicast,
+            safi: Safi::UNICAST,
             prefixes: vec![prefix.clone()],
         })],
         announced_routes: vec![],

crates/integration-tests/tests/collision.rs

@@ -40,7 +40,7 @@ async fn server_dialing(listener: &TcpListener) -> Result<TestServer> {
             remote_asn: PEER_ASN,
             hold_time_secs: 90,
             local_address: None,
-            families: FamilySet::ALL,
+            families: FamilySet::supported(),
         }];
     })
     .await

crates/integration-tests/tests/dialer.rs

@@ -31,7 +31,7 @@ async fn server_dialing(listener: &TcpListener, remote_asn: u32) -> Result<TestS
             remote_asn,
             hold_time_secs: 90,
             local_address: None,
-            families: FamilySet::ALL,
+            families: FamilySet::supported(),
         }];
     })
     .await
@@ -184,7 +184,7 @@ async fn test_inbound_from_configured_address_uses_config() -> Result<()> {
             remote_asn: PEER_ASN,
             hold_time_secs: 90,
             local_address: None,
-            families: FamilySet::ALL,
+            families: FamilySet::supported(),
         }];
     })
     .await?;
@@ -231,7 +231,7 @@ async fn test_local_address_is_used_as_the_source() -> Result<()> {
             remote_asn: PEER_ASN,
             hold_time_secs: 90,
             local_address: Some(Ipv4Addr::LOCALHOST.into()),
-            families: FamilySet::ALL,
+            families: FamilySet::supported(),
         }];
     })
     .await?;
@@ -247,7 +247,7 @@ async fn test_local_address_is_used_as_the_source() -> Result<()> {
             remote_asn: PEER_ASN,
             hold_time_secs: 90,
             local_address: Some(Ipv4Addr::new(192, 0, 2, 1).into()),
-            families: FamilySet::ALL,
+            families: FamilySet::supported(),
         }];
     })
     .await?;

crates/integration-tests/tests/interop.rs

@@ -7,7 +7,7 @@ use bgp_integration_tests::test_peer::TestPeer;
 use bgp_integration_tests::test_server::TestServer;
 use bgp_packet::message::{Message, PathAttribute};
 use bgp_packet::open::{Capability, OpenOption};
-use bgp_primitives::constants::{AddressFamilyId, SubsequentAfi};
+use bgp_primitives::constants::{Afi, Safi};
 use bgp_primitives::ip_prefix::IpPrefix;
 use bgp_rib::{LocalRoute, LocalRouteAttrs};
 use eyre::Result;
@@ -61,9 +61,9 @@ async fn test_server_advertises_ipv4_and_ipv6_unicast() -> Result<()> {
             OpenOption::Capabilities(c) => c.iter(),
         })
         .collect();
-    for afi in [AddressFamilyId::Ipv4, AddressFamilyId::Ipv6] {
+    for afi in [Afi::IPV4, Afi::IPV6] {
         assert!(
-            caps.iter().any(|c| matches!(c, Capability::MultiProtocol { afi: a, safi: SubsequentAfi::Unicast } if *a == afi)),
+            caps.iter().any(|c| matches!(c, Capability::MultiProtocol { afi: a, safi: Safi::UNICAST } if *a == afi)),
             "missing MP capability for {afi:?}: {caps:?}"
         );
     }
@@ -133,7 +133,7 @@ async fn test_ipv6_prefix_update_shape() -> Result<()> {
             .path_attributes
             .iter()
             .any(|a| matches!(a, PathAttribute::MpReachNlri(m)
-        if m.afi == AddressFamilyId::Ipv6 && m.prefixes[0].to_string() == "2001:db8:1::/48"))
+        if m.afi == Afi::IPV6 && m.prefixes[0].to_string() == "2001:db8:1::/48"))
     );
     let _ = Ipv4Addr::LOCALHOST;
     Ok(())

crates/integration-tests/tests/negotiation.rs

@@ -12,11 +12,11 @@ use bgp_packet::message::{
     OriginPathAttribute, PathAttribute, UpdateMessage,
 };
 use bgp_packet::open::Capability;
-use bgp_primitives::constants::{AddressFamilyId, SubsequentAfi};
+use bgp_primitives::constants::{Afi, AfiSafi, Safi};
 use bgp_primitives::ip_prefix::IpPrefix;
 use bgp_rib::{LocalRoute, LocalRouteAttrs};
 use bgp_server::config::PeerConfig;
-use bgp_server::family::{AddressFamily, FamilySet};
+use bgp_server::family::{FamilySet, SUPPORTED};
 use eyre::{Result, bail};
 
 const LOCAL_ASN: u32 = 65000;
@@ -28,7 +28,7 @@ fn prefix(s: &str) -> IpPrefix {
     IpPrefix::try_from(s).unwrap()
 }
 
-fn only(family: AddressFamily) -> FamilySet {
+fn only(family: AfiSafi) -> FamilySet {
     [family].into_iter().collect()
 }
 
@@ -58,8 +58,8 @@ fn dual_stack_update() -> UpdateMessage {
             AsPathAttribute::from_asns(vec![PEER_ASN]),
             PathAttribute::NextHop(NextHopPathAttribute(Ipv4Addr::new(192, 0, 2, 1))),
             PathAttribute::MpReachNlri(MpReachNlriPathAttribute {
-                afi: AddressFamilyId::Ipv6,
+                afi: Afi::IPV6,
-                safi: SubsequentAfi::Unicast,
+                safi: Safi::UNICAST,
                 next_hop: NlriNextHop::Ipv6(Ipv6Addr::new(0x2001, 0xdb8, 0, 0, 0, 0, 0, 1)),
                 prefixes: vec![prefix("2001:db8:1::/48")],
             }),
@@ -79,18 +79,22 @@ async fn test_families_are_the_ones_both_sides_offered() -> Result<()> {
             PEER_ASN,
             PEER_ID,
             90,
-            vec![four_octet(), AddressFamily::Ipv4Unicast.capability()],
+            vec![
+                four_octet(),
+                Capability::multiprotocol(AfiSafi::IPV4_UNICAST),
+            ],
         )
         .await?;
 
-    for family in AddressFamily::ALL {
+    for family in SUPPORTED {
         assert!(
-            open.capabilities().any(|c| *c == family.capability()),
+            open.capabilities()
+                .any(|c| *c == Capability::multiprotocol(family)),
             "the server's OPEN offers {family}"
         );
     }
     let session = wait_for_session(&server).await?;
-    assert_eq!(session.families, only(AddressFamily::Ipv4Unicast));
+    assert_eq!(session.families, only(AfiSafi::IPV4_UNICAST));
     Ok(())
 }
 
@@ -104,7 +108,10 @@ async fn test_routes_of_a_family_not_negotiated_are_not_accepted() -> Result<()>
         PEER_ASN,
         PEER_ID,
         90,
-        vec![four_octet(), AddressFamily::Ipv4Unicast.capability()],
+        vec![
+            four_octet(),
+            Capability::multiprotocol(AfiSafi::IPV4_UNICAST),
+        ],
     )
     .await?;
     wait_for_session(&server).await?;
@@ -146,11 +153,14 @@ async fn test_routes_of_a_family_not_negotiated_are_not_sent() -> Result<()> {
             PEER_ASN,
             PEER_ID,
             90,
-            vec![four_octet(), AddressFamily::Ipv6Unicast.capability()],
+            vec![
+                four_octet(),
+                Capability::multiprotocol(AfiSafi::IPV6_UNICAST),
+            ],
         )
         .await?;
     let session = wait_for_session(&server).await?;
-    assert_eq!(session.families, only(AddressFamily::Ipv6Unicast));
+    assert_eq!(session.families, only(AfiSafi::IPV6_UNICAST));
 
     // A second peer that takes IPv4 is sent the route. The advertiser
     // handles sessions in the order they came up, so by the time this one
@@ -178,7 +188,7 @@ async fn test_no_family_in_common_refuses_the_session() -> Result<()> {
             remote_asn: PEER_ASN,
             hold_time_secs: 90,
             local_address: None,
-            families: only(AddressFamily::Ipv6Unicast),
+            families: only(AfiSafi::IPV6_UNICAST),
         }];
         // Keep the dialer out of the way.
         c.connect_retry = Duration::from_secs(3600);
@@ -190,7 +200,10 @@ async fn test_no_family_in_common_refuses_the_session() -> Result<()> {
         PEER_ASN,
         PEER_ID,
         90,
-        vec![four_octet(), AddressFamily::Ipv4Unicast.capability()],
+        vec![
+            four_octet(),
+            Capability::multiprotocol(AfiSafi::IPV4_UNICAST),
+        ],
     )
     .await?;
     let open = peer.recv_open().await?;
@@ -198,7 +211,7 @@ async fn test_no_family_in_common_refuses_the_session() -> Result<()> {
         .capabilities()
         .filter(|c| matches!(c, Capability::MultiProtocol { .. }))
         .collect();
-    assert_eq!(offered, [&AddressFamily::Ipv6Unicast.capability()]);
+    assert_eq!(offered, [&Capability::multiprotocol(AfiSafi::IPV6_UNICAST)]);
 
     match tokio::time::timeout(TIMEOUT, peer.recv()).await?? {
         Message::Notification(n) => {
@@ -215,10 +228,10 @@ async fn test_no_family_in_common_refuses_the_session() -> Result<()> {
     Ok(())
 }
 
-/// Real routers offer families we have never heard of. That must not stop
+/// Real routers offer families we do not support. That must not stop
 /// the families we do share from working.
 #[tokio::test]
-async fn test_a_peer_offering_an_unknown_family_is_still_accepted() -> Result<()> {
+async fn test_a_peer_offering_an_unsupported_family_is_still_accepted() -> Result<()> {
     let server = TestServer::start(LOCAL_ASN).await?;
     let mut peer = TestPeer::connect(server.addr).await?;
     peer.handshake_with(
@@ -227,19 +240,14 @@ async fn test_a_peer_offering_an_unknown_family_is_still_accepted() -> Result<()
         90,
         vec![
             four_octet(),
-            // Multiprotocol for L2VPN EVPN (AFI 25, SAFI 70).
+            Capability::multiprotocol(AfiSafi::new(Afi::L2VPN, Safi::EVPN)),
-            Capability::UnknownCapability {
+            Capability::multiprotocol(AfiSafi::IPV4_UNICAST),
-                type_code: 1,
-                length: 4,
-                value: vec![0, 25, 0, 70],
-            },
-            AddressFamily::Ipv4Unicast.capability(),
         ],
     )
     .await?;
 
     let session = wait_for_session(&server).await?;
-    assert_eq!(session.families, only(AddressFamily::Ipv4Unicast));
+    assert_eq!(session.families, only(AfiSafi::IPV4_UNICAST));
     assert!(
         session.four_octet_asn,
         "capabilities after it are still read"
@@ -265,7 +273,7 @@ async fn test_a_peer_without_capabilities_gets_two_octet_as_paths() -> Result<()
     peer.handshake_with(PEER_ASN, PEER_ID, 90, vec![]).await?;
     let session = wait_for_session(&server).await?;
     assert!(!session.four_octet_asn);
-    assert_eq!(session.families, only(AddressFamily::Ipv4Unicast));
+    assert_eq!(session.families, only(AfiSafi::IPV4_UNICAST));
 
     // The test peer now decodes AS_PATH two octets wide, as such a speaker
     // would; an UPDATE encoded four octets wide would not parse.

crates/packet/src/message.rs

@@ -14,8 +14,7 @@ use serde::{Deserialize, Serialize};
 use serde_repr::{Deserialize_repr, Serialize_repr};
 use strum::EnumDiscriminants;
 
-use bgp_primitives::constants::AddressFamilyId;
+use bgp_primitives::constants::{AddressFamilyId, Afi, AfiSafi, Safi};
-use bgp_primitives::constants::SubsequentAfi;
 use bgp_primitives::ip_prefix::IpPrefix;
 use bgp_primitives::parser::BgpParserError;
 use bgp_primitives::parser::EncodeError;
@@ -120,8 +119,6 @@ impl Message {
                             ProtocolErrorValues::UnknownOpenOption(_) => {
                                 OpenMsgSubcode::UnsupportedOptionalParam
                             }
-                            // Handled inside the capability parser.
-                            ProtocolErrorValues::UnknownAddressFamily => OpenMsgSubcode::Unspecific,
                         },
                         _ => OpenMsgSubcode::Unspecific,
                     };
@@ -1415,80 +1412,85 @@ fn parse_prefix(
     Ok((buf, prefix))
 }
 
+/// The IP version of the prefixes that make up the NLRI of `family`.
+///
+/// Any family can be named in MP_REACH_NLRI and MP_UNREACH_NLRI, but only
+/// unicast and multicast routes of IPv4 and IPv6 are plain prefixes, and
+/// those are the only NLRI this parser reads. A peer does not send routes
+/// of a family that was not negotiated, so the others do not come up.
+fn ip_version_of_nlri<'a>(
+    family: AfiSafi,
+) -> Result<AddressFamilyId, nom::Err<BgpParserError<&'a [u8]>>> {
+    let unsupported = || {
+        Failure(BgpParserError::Malformed(format!(
+            "NLRI of {family} is not supported"
+        )))
+    };
+    if family.safi != Safi::UNICAST && family.safi != Safi::MULTICAST {
+        return Err(unsupported());
+    }
+    AddressFamilyId::try_from(family.afi).map_err(|_| unsupported())
+}
+
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
 pub struct MpReachNlriPathAttribute {
-    pub afi: AddressFamilyId,
+    pub afi: Afi,
-    pub safi: SubsequentAfi,
+    pub safi: Safi,
     /// Next hop address (either IPv4 or IPv6 for now).
     pub next_hop: NlriNextHop,
     pub prefixes: Vec<IpPrefix>,
 }
 
 impl MpReachNlriPathAttribute {
+    pub fn family(&self) -> AfiSafi {
+        AfiSafi::new(self.afi, self.safi)
+    }
+
     pub fn from_wire<'a>(
         _: &ParserContext,
         buf: &'a [u8],
     ) -> IResult<&'a [u8], Self, BgpParserError<&'a [u8]>> {
-        let (buf, raw_afi) = be_u16(buf)?;
+        let (buf, afi) = be_u16(buf).map(|(buf, afi)| (buf, Afi(afi)))?;
-        let afi = AddressFamilyId::try_from(raw_afi)
+        let (buf, safi) = be_u8(buf).map(|(buf, safi)| (buf, Safi(safi)))?;
-            .map_err(|e| Failure(BgpParserError::Malformed(e.to_string())))?;
+        let version = ip_version_of_nlri(AfiSafi::new(afi, safi))?;
-
-        let (buf, raw_safi) = be_u8(buf)?;
-        let safi = SubsequentAfi::try_from(raw_safi)
-            .map_err(|e| Failure(BgpParserError::Malformed(e.to_string())))?;
 
         let (buf, nh_len) = be_u8(buf)?;
-
+        let (buf, nh_bytes) = nom::bytes::take(usize::from(nh_len)).parse(buf)?;
-        let (buf, next_hop, prefixes) = match afi {
+        let address = |bytes: &[u8]| {
-            AddressFamilyId::Ipv4 => {
+            let mut octets = [0u8; 16];
-                // Read the length of the nexthop which should equal 4.
+            octets.copy_from_slice(bytes);
-                if nh_len != 4 {
+            Ipv6Addr::from(octets)
-                    return Err(Failure(BgpParserError::Malformed(format!(
+        };
-                        "Got nexthop address length {} when expected 4 for IPv4 AFI",
+        let next_hop = match (version, nh_bytes.len()) {
-                        nh_len
+            (AddressFamilyId::Ipv4, 4) => NlriNextHop::Ipv4(Ipv4Addr::new(
-                    ))));
+                nh_bytes[0],
-                }
+                nh_bytes[1],
-                // Read the nexthop address which should now be an IPv4 address.
+                nh_bytes[2],
-                let (buf, nh_bytes) = be_u32(buf)?;
+                nh_bytes[3],
-                let next_hop = NlriNextHop::Ipv4(Ipv4Addr::from(nh_bytes));
+            )),
-
+            (AddressFamilyId::Ipv4, other) => {
-                let (buf, prefixes) =
+                return Err(Failure(BgpParserError::Malformed(format!(
-                    nom::multi::many0(|buf| parse_prefix(AddressFamilyId::Ipv4, buf)).parse(buf)?;
+                    "Got nexthop address length {other} when expected 4 for IPv4 AFI"
-                (buf, next_hop, prefixes)
+                ))));
             }
-            AddressFamilyId::Ipv6 => {
+            // RFC 2545: a global address, optionally followed by a
-                // https://datatracker.ietf.org/doc/html/rfc2545 defines that the nexthop address may be 16 or 32 bytes long.
+            // link-local one.
-                let (buf, nh_bytes) = nom::bytes::take(nh_len as usize).parse(buf)?;
+            (AddressFamilyId::Ipv6, 16) => NlriNextHop::Ipv6(address(nh_bytes)),
-                let nexthop = match nh_bytes.len() {
+            (AddressFamilyId::Ipv6, 32) => NlriNextHop::Ipv6WithLl {
-                    16 => {
+                global: address(&nh_bytes[..16]),
-                        // unwrap should never fire since we have explicitly checked the length.
+                link_local: address(&nh_bytes[16..]),
-                        let slice: [u8; 16] = nh_bytes.try_into().unwrap();
+            },
-                        NlriNextHop::Ipv6(Ipv6Addr::from(slice))
+            (AddressFamilyId::Ipv6, other) => {
-                    }
+                return Err(Failure(BgpParserError::Malformed(format!(
-                    32 => {
+                    "Mismatched IPv6 nexthop length, got {other}, want 16 or 32"
-                        // unwrap should never fire since we have explicitly checked the length.
+                ))));
-                        let slice: [u8; 32] = nh_bytes.try_into().unwrap();
-                        let global_bytes: [u8; 16] = slice[0..16].try_into().unwrap();
-                        let global = Ipv6Addr::from(global_bytes);
-                        let link_local_bytes: [u8; 16] = slice[16..32].try_into().unwrap();
-                        let link_local = Ipv6Addr::from(link_local_bytes);
-                        NlriNextHop::Ipv6WithLl { global, link_local }
-                    }
-                    _ => {
-                        return Err(Failure(BgpParserError::Malformed(format!(
-                            "Mismatched IPv6 nexthop length, got {}, want 16 or 32",
-                            nh_bytes.len()
-                        ))));
-                    }
-                };
-                // Reserved 0 byte (formerly SNPA).
-                let (buf, _) = be_u8(buf)?;
-                let (buf, prefixes) =
-                    nom::multi::many0(|buf| parse_prefix(AddressFamilyId::Ipv6, buf)).parse(buf)?;
-                (buf, nexthop, prefixes)
             }
         };
 
+        // Reserved byte (formerly the SNPA count), for every family.
+        let (buf, _) = be_u8(buf)?;
+        let (buf, prefixes) = nom::multi::many0(|buf| parse_prefix(version, buf)).parse(buf)?;
+
         Ok((
             buf,
             Self {
@@ -1501,8 +1503,8 @@ impl MpReachNlriPathAttribute {
     }
 
     pub fn to_wire(&self, ctx: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> {
-        out.put_u16(self.afi as u16);
+        out.put_u16(self.afi.0);
-        out.put_u8(self.safi as u8);
+        out.put_u8(self.safi.0);
         out.put_u8(self.next_hop.wire_len());
         self.next_hop.to_wire(ctx, out)?;
         out.put_u8(0);
@@ -1527,25 +1529,25 @@ impl MpReachNlriPathAttribute {
 
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
 pub struct MpUnreachNlriPathAttribute {
-    pub afi: AddressFamilyId,
+    pub afi: Afi,
-    pub safi: SubsequentAfi,
+    pub safi: Safi,
     pub prefixes: Vec<IpPrefix>,
 }
 
 impl MpUnreachNlriPathAttribute {
+    pub fn family(&self) -> AfiSafi {
+        AfiSafi::new(self.afi, self.safi)
+    }
+
     pub fn from_wire<'a>(
         _: &ParserContext,
         buf: &'a [u8],
     ) -> IResult<&'a [u8], Self, BgpParserError<&'a [u8]>> {
-        let (buf, raw_afi) = be_u16(buf)?;
+        let (buf, afi) = be_u16(buf).map(|(buf, afi)| (buf, Afi(afi)))?;
-        let afi = AddressFamilyId::try_from(raw_afi)
+        let (buf, safi) = be_u8(buf).map(|(buf, safi)| (buf, Safi(safi)))?;
-            .map_err(|e| Failure(BgpParserError::Malformed(e.to_string())))?;
+        let version = ip_version_of_nlri(AfiSafi::new(afi, safi))?;
 
-        let (buf, raw_safi) = be_u8(buf)?;
+        let (buf, prefixes) = nom::multi::many0(|buf| parse_prefix(version, buf)).parse(buf)?;
-        let safi = SubsequentAfi::try_from(raw_safi)
-            .map_err(|e| Failure(BgpParserError::Malformed(e.to_string())))?;
-
-        let (buf, prefixes) = nom::multi::many0(|buf| parse_prefix(afi, buf)).parse(buf)?;
 
         Ok((
             buf,
@@ -1558,8 +1560,8 @@ impl MpUnreachNlriPathAttribute {
     }
 
     pub fn to_wire(&self, _: &ParserContext, out: &mut BytesMut) -> Result<(), EncodeError> {
-        out.put_u16(self.afi as u16);
+        out.put_u16(self.afi.0);
-        out.put_u8(self.safi as u8);
+        out.put_u8(self.safi.0);
         for prefix in &self.prefixes {
             out.put_u8(prefix.length);
             out.put(&prefix.prefix[..]);
@@ -1639,7 +1641,7 @@ mod tests {
     };
 
     use bgp_primitives::{
-        constants::{AddressFamilyId, SubsequentAfi},
+        constants::{AddressFamilyId, Afi, Safi},
         ip_prefix::IpPrefix,
         parser::ParserContext,
     };
@@ -1788,8 +1790,8 @@ mod tests {
             0x01, 0x0d, 0xb8
         ],
         PathAttribute::MpReachNlri(MpReachNlriPathAttribute {
-            afi: AddressFamilyId::Ipv6,
+            afi: Afi::IPV6,
-            safi: SubsequentAfi::Unicast,
+            safi: Safi::UNICAST,
             next_hop: NlriNextHop::Ipv6WithLl {
                 global: ipv6!("2001:db8::1"),
                 link_local: ipv6!("fe80::12"),
@@ -1809,8 +1811,8 @@ mod tests {
             0x80, 0x0f, 0x08, 0x00, 0x02, 0x01, 0x20, 0x20, 0x01, 0x0d, 0xb8
         ],
         PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute {
-            afi: AddressFamilyId::Ipv6,
+            afi: Afi::IPV6,
-            safi: SubsequentAfi::Unicast,
+            safi: Safi::UNICAST,
             prefixes: vec![IpPrefix {
                 address_family: AddressFamilyId::Ipv6,
                 prefix: vec![0x20, 0x01, 0x0d, 0xb8],
@@ -2025,6 +2027,53 @@ mod tests {
     );
 
     // Rejects trailing garbage after the NLRI section (a partial prefix).
+    /// The reserved octet after the next hop is there for every family. It
+    /// used to be skipped only for IPv6, so an IPv4 MP_REACH_NLRI gained a
+    /// 0.0.0.0/0 route that the peer never sent.
+    #[test]
+    fn mp_reach_for_ipv4_has_no_phantom_default_route() {
+        let ctx = default_v6_context();
+        #[rustfmt::skip]
+        let wire = [
+            0x00, 0x01, 0x01,             // IPv4 unicast
+            0x04, 192, 0, 2, 1,           // next hop
+            0x00,                         // reserved
+            0x18, 10, 1, 2,               // 10.1.2.0/24
+        ];
+        let (rest, parsed) = MpReachNlriPathAttribute::from_wire(&ctx, &wire).unwrap();
+        assert!(rest.is_empty());
+        assert_eq!(
+            parsed.family(),
+            bgp_primitives::constants::AfiSafi::IPV4_UNICAST
+        );
+        assert_eq!(
+            parsed.next_hop,
+            NlriNextHop::Ipv4(Ipv4Addr::new(192, 0, 2, 1))
+        );
+        assert_eq!(parsed.prefixes, vec![v4_prefix(24, &[10, 1, 2])]);
+
+        let mut out = BytesMut::new();
+        parsed.to_wire(&ctx, &mut out).unwrap();
+        assert_eq!(&out[..], &wire[..]);
+    }
+
+    /// Any family can be named, but only IP prefixes can be read as NLRI.
+    /// Reading the routes of another family as prefixes would invent routes.
+    #[test]
+    fn nlri_of_a_family_that_is_not_ip_prefixes_is_refused() {
+        let ctx = default_v6_context();
+        // L2VPN EVPN, and IPv4 with labels: neither is a list of prefixes.
+        for family in [[0x00, 0x19, 0x46], [0x00, 0x01, 0x04]] {
+            let mut reach = family.to_vec();
+            reach.extend_from_slice(&[0x04, 192, 0, 2, 1, 0x00, 0x18, 10, 1, 2]);
+            assert!(MpReachNlriPathAttribute::from_wire(&ctx, &reach).is_err());
+
+            let mut unreach = family.to_vec();
+            unreach.extend_from_slice(&[0x18, 10, 1, 2]);
+            assert!(MpUnreachNlriPathAttribute::from_wire(&ctx, &unreach).is_err());
+        }
+    }
+
     /// AS_PATH is as wide as the session negotiated; AS4_PATH is always
     /// four octets wide (RFC 6793).
     #[test]

crates/packet/src/open.rs

@@ -13,7 +13,7 @@ use crate::errors::InvalidValue;
 use strum::EnumDiscriminants;
 
 use bgp_primitives::{
-    constants::{AddressFamilyId, SubsequentAfi},
+    constants::{Afi, AfiSafi, Safi},
     parser::{BgpParserError, EncodeError, ParserContext, ProtocolErrorValues},
 };
 
@@ -222,30 +222,11 @@ impl OpenOption {
     }
 }
 
-/// An AFI in a capability, or the marker that makes the capability unknown.
-fn known_afi<'a>(afi: u16) -> Result<AddressFamilyId, nom::Err<BgpParserError<&'a [u8]>>> {
-    AddressFamilyId::try_from(afi).map_err(|_| unknown_family())
-}
-
-/// A SAFI in a capability, or the marker that makes the capability unknown.
-fn known_safi<'a, T>(safi: T) -> Result<SubsequentAfi, nom::Err<BgpParserError<&'a [u8]>>>
-where
-    SubsequentAfi: TryFrom<T>,
-{
-    SubsequentAfi::try_from(safi).map_err(|_| unknown_family())
-}
-
-fn unknown_family<'a>() -> nom::Err<BgpParserError<&'a [u8]>> {
-    Failure(BgpParserError::ProtocolError(
-        ProtocolErrorValues::UnknownAddressFamily,
-    ))
-}
-
 /// One address family in a Multiple Labels capability (RFC 8277 §2.1).
 #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)]
 pub struct MultiLabelEntry {
-    pub afi: u16,
+    pub afi: Afi,
-    pub safi: u8,
+    pub safi: Safi,
     /// Maximum number of labels in one NLRI of this family.
     pub count: u8,
 }
@@ -255,16 +236,16 @@ pub struct MultiLabelEntry {
 pub enum Capability {
     /// MultiProtocol Extension (RFC 2858).
     MultiProtocol {
-        afi: AddressFamilyId,
+        afi: Afi,
-        safi: SubsequentAfi,
+        safi: Safi,
     } = 1,
     /// Route Refresh capability (RFC 2918).
     RouteRefresh = 2,
     /// Outbound Route Filtering (RFC 5291).
     /// https://datatracker.ietf.org/doc/html/rfc5291
     OutboundRouteFilter {
-        afi: AddressFamilyId,
+        afi: Afi,
-        safi: SubsequentAfi,
+        safi: Safi,
         orfs: Vec<OutboundRouteFilterEntry>,
     } = 3,
     /// Extended Next Hop encoding (RFC 8950).
@@ -276,7 +257,7 @@ pub enum Capability {
     /// BGPSec (RFC 8205).
     BgpSec {
         header: BgpSecHeader,
-        afi: AddressFamilyId,
+        afi: Afi,
     } = 7,
     /// Multiple labels (RFC 8277 §2.1): for each address family, how many
     /// labels the speaker can handle in one NLRI.
@@ -311,8 +292,8 @@ pub enum Capability {
     } = 65,
     /// Additional Path (RFC 7911).
     AddPath {
-        afi: AddressFamilyId,
+        afi: Afi,
-        safi: SubsequentAfi,
+        safi: Safi,
         send_recv: AddPathSendRecv,
     } = 69,
     /// Enhanced Route Refresh (RFC 7313).
@@ -350,12 +331,19 @@ impl Capability {
             Capability::UnknownCapability { type_code, .. } => *type_code,
         }
     }
+    /// The Multiprotocol capability that offers `family` to a peer.
+    pub fn multiprotocol(family: AfiSafi) -> Self {
+        Self::MultiProtocol {
+            afi: family.afi,
+            safi: family.safi,
+        }
+    }
+
     /// `from_wire` parses a Capability message.
     ///
-    /// A capability we know, about an address family we do not, is kept as
+    /// Address families are read as numbers, whatever they are: a peer is
-    /// an [`Capability::UnknownCapability`]: a peer is free to advertise
+    /// free to advertise families we have never heard of, and RFC 5492 §3
-    /// families we have never heard of, and RFC 5492 §3 has us ignore what
+    /// has us ignore what we do not support rather than refuse the OPEN.
-    /// we do not support rather than refuse the OPEN.
     pub fn from_wire<'a>(
         _ctx: &ParserContext,
         buf: &'a [u8],
@@ -365,17 +353,7 @@ impl Capability {
         // `cap_buf` holds exactly this capability's value; `buf` is what
         // follows it and is what the caller's list parser continues with.
         let (buf, cap_buf) = nom::bytes::take(len).parse(buf)?;
-        let capability = match Self::parse_value(typ, cap_buf) {
+        let capability = Self::parse_value(typ, cap_buf)?;
-            Ok(capability) => capability,
-            Err(Failure(BgpParserError::ProtocolError(
-                ProtocolErrorValues::UnknownAddressFamily,
-            ))) => Self::UnknownCapability {
-                type_code: typ,
-                length: len,
-                value: cap_buf.to_vec(),
-            },
-            Err(e) => return Err(e),
-        };
         Ok((buf, capability))
     }
 
@@ -402,8 +380,8 @@ impl Capability {
                 let (cap_buf, _) = be_u8(cap_buf)?;
                 let (cap_buf, safi) = be_u8(cap_buf)?;
                 require_empty!(cap_buf, "MultiProtocol");
-                let afi = known_afi(afi)?;
+                let afi = Afi(afi);
-                let safi = known_safi(safi)?;
+                let safi = Safi(safi);
                 Ok(Self::MultiProtocol { afi, safi })
             }
             v if v == CapabilityDiscriminants::RouteRefresh as u8 => {
@@ -414,8 +392,8 @@ impl Capability {
                 let (cap_buf, afi) = be_u16(cap_buf)?;
                 let (cap_buf, _) = be_u8(cap_buf)?; // Move past reverved byte.
                 let (cap_buf, safi) = be_u8(cap_buf)?;
-                let afi = known_afi(afi)?;
+                let afi = Afi(afi);
-                let safi = known_safi(safi)?;
+                let safi = Safi(safi);
 
                 let (cap_buf, _num_orfs) = be_u8(cap_buf)?;
 
@@ -454,9 +432,13 @@ impl Capability {
                     let (buf, afi) = be_u16(buf)?;
                     let (buf, safi) = be_u16(buf)?;
                     let (buf, nh_afi) = be_u16(buf)?;
-                    let afi = known_afi(afi)?;
+                    let afi = Afi(afi);
-                    let safi = known_safi(safi)?;
+                    // Two octets on the wire here (RFC 8950), one everywhere
-                    let nh_afi = known_afi(nh_afi)?;
+                    // else: no SAFI is larger than 255.
+                    let safi = u8::try_from(safi)
+                        .map(Safi)
+                        .map_err(|_| fail!("SAFI {} does not fit in one octet", safi))?;
+                    let nh_afi = Afi(nh_afi);
                     Ok((buf, ExtendedNextHop { afi, safi, nh_afi }))
                 };
                 let (cap_buf, entries) = nom::multi::many1(parse_enh).parse(cap_buf)?;
@@ -471,7 +453,7 @@ impl Capability {
             v if v == CapabilityDiscriminants::BgpSec as u8 => {
                 let (cap_buf, header) = be_u8(cap_buf)?;
                 let (cap_buf, afi) = be_u16(cap_buf)?;
-                let afi = known_afi(afi)?;
+                let afi = Afi(afi);
                 require_empty!(cap_buf, "BgpSec");
                 Ok(Self::BgpSec {
                     header: BgpSecHeader(header),
@@ -479,8 +461,6 @@ impl Capability {
                 })
             }
             v if v == CapabilityDiscriminants::MultiLabelCompat as u8 => {
-                // The AFI and SAFI are kept as numbers: this capability is
-                // used with labelled families we have no named constant for.
                 let parse_entry = |buf: &'a [u8]| -> IResult<
                     &'a [u8],
                     MultiLabelEntry,
@@ -489,7 +469,14 @@ impl Capability {
                     let (buf, afi) = be_u16(buf)?;
                     let (buf, safi) = be_u8(buf)?;
                     let (buf, count) = be_u8(buf)?;
-                    Ok((buf, MultiLabelEntry { afi, safi, count }))
+                    Ok((
+                        buf,
+                        MultiLabelEntry {
+                            afi: Afi(afi),
+                            safi: Safi(safi),
+                            count,
+                        },
+                    ))
                 };
                 let (cap_buf, entries) = nom::multi::many1(parse_entry).parse(cap_buf)?;
                 require_empty!(cap_buf, "MultiLabelCompat");
@@ -514,8 +501,8 @@ impl Capability {
                     let (buf, afi) = be_u16(buf)?;
                     let (buf, safi) = be_u8(buf)?;
                     let (buf, flags) = be_u8(buf)?;
-                    let afi = known_afi(afi)?;
+                    let afi = Afi(afi);
-                    let safi = known_safi(safi)?;
+                    let safi = Safi(safi);
                     Ok((
                         buf,
                         GracefulRestartEntry {
@@ -542,8 +529,8 @@ impl Capability {
                 let (cap_buf, afi) = be_u16(cap_buf)?;
                 let (cap_buf, safi) = be_u8(cap_buf)?;
                 let (cap_buf, send_recv) = be_u8(cap_buf)?;
-                let afi = known_afi(afi)?;
+                let afi = Afi(afi);
-                let safi = known_safi(safi)?;
+                let safi = Safi(safi);
                 let send_recv = AddPathSendRecv::try_from(send_recv).map_err(|e| fail!("{}", e))?;
                 require_empty!(cap_buf, "AddPath");
                 Ok(Self::AddPath {
@@ -566,8 +553,8 @@ impl Capability {
                     let (buf, safi) = be_u8(buf)?;
                     let (buf, flags) = be_u8(buf)?;
                     let (buf, stale_time) = be_u24(buf)?;
-                    let afi = known_afi(afi)?;
+                    let afi = Afi(afi);
-                    let safi = known_safi(safi)?;
+                    let safi = Safi(safi);
                     Ok((
                         buf,
                         LongLivedGracefulRestart {
@@ -599,15 +586,15 @@ impl Capability {
         out.put_u8(self.payload_len());
         match self {
             Capability::MultiProtocol { afi, safi } => {
-                out.put_u16(*afi as u16);
+                out.put_u16(afi.0);
                 out.put_u8(0); // Reserved byte.
-                out.put_u8(*safi as u8);
+                out.put_u8(safi.0);
             }
             Capability::RouteRefresh => {}
             Capability::OutboundRouteFilter { afi, safi, orfs } => {
-                out.put_u16(*afi as u16);
+                out.put_u16(afi.0);
                 out.put_u8(0); // Reserved byte.
-                out.put_u8(*safi as u8);
+                out.put_u8(safi.0);
                 for orf in orfs {
                     out.put_u8(orf.r#type.clone().into());
                     out.put_u8(orf.send_recv.clone() as u8);
@@ -615,20 +602,20 @@ impl Capability {
             }
             Capability::ExtendedNextHop { entries } => {
                 for entry in entries {
-                    out.put_u16(entry.afi as u16);
+                    out.put_u16(entry.afi.0);
-                    out.put_u16(entry.safi as u16);
+                    out.put_u16(u16::from(entry.safi.0));
-                    out.put_u16(entry.nh_afi as u16);
+                    out.put_u16(entry.nh_afi.0);
                 }
             }
             Capability::ExtendedMessage => {}
             Capability::BgpSec { header, afi } => {
                 out.put_u8(header.0);
-                out.put_u16(*afi as u16);
+                out.put_u16(afi.0);
             }
             Capability::MultiLabelCompat { entries } => {
                 for entry in entries {
-                    out.put_u16(entry.afi);
+                    out.put_u16(entry.afi.0);
-                    out.put_u8(entry.safi);
+                    out.put_u8(entry.safi.0);
                     out.put_u8(entry.count);
                 }
             }
@@ -654,8 +641,8 @@ impl Capability {
                 flags_restart_time |= restart_time;
                 out.put_u16(flags_restart_time);
                 for entry in entries {
-                    out.put_u16(entry.afi as u16);
+                    out.put_u16(entry.afi.0);
-                    out.put_u8(entry.safi as u8);
+                    out.put_u8(entry.safi.0);
                     let mut flags = 0;
                     if entry.preserve_forwarding {
                         flags |= 1 << 7;
@@ -671,15 +658,15 @@ impl Capability {
                 safi,
                 send_recv,
             } => {
-                out.put_u16(*afi as u16);
+                out.put_u16(afi.0);
-                out.put_u8(*safi as u8);
+                out.put_u8(safi.0);
                 out.put_u8(send_recv.into());
             }
             Capability::EnhancedRouteRefresh => {}
             Capability::LongLivedGracefulRestart(llgrs) => {
                 for llgr in llgrs {
-                    out.put_u16(llgr.afi as u16);
+                    out.put_u16(llgr.afi.0);
-                    out.put_u8(llgr.safi as u8);
+                    out.put_u8(llgr.safi.0);
                     out.put_u8(llgr.flags);
                     out.put(&llgr.stale_time.to_be_bytes()[1..]);
                 }
@@ -823,11 +810,11 @@ impl From<&AddPathSendRecv> for u8 {
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
 pub struct ExtendedNextHop {
     /// The Address Family to update the nexthop format for.
-    pub afi: AddressFamilyId,
+    pub afi: Afi,
     /// The Subsequent Address Family to update the nexthop format for.
-    pub safi: SubsequentAfi,
+    pub safi: Safi,
     /// The Address Family that nexthops will be encoded as.
-    pub nh_afi: AddressFamilyId,
+    pub nh_afi: Afi,
 }
 
 #[derive(Debug, Clone, Serialize, Deserialize, EnumDiscriminants, PartialEq, Eq)]
@@ -869,8 +856,8 @@ impl From<&BgpRole> for u8 {
 
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
 pub struct LongLivedGracefulRestart {
-    pub afi: AddressFamilyId,
+    pub afi: Afi,
-    pub safi: SubsequentAfi,
+    pub safi: Safi,
     pub flags: u8,
     /// stale_time is the time in seconds after which routes are considered stale.
     /// Note that this is represented as a u24 on the wire.
@@ -889,8 +876,8 @@ bitfield! {
 /// Represents an entry in the BGP Graceful Restart capability.
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
 pub struct GracefulRestartEntry {
-    afi: AddressFamilyId,
+    afi: Afi,
-    safi: SubsequentAfi,
+    safi: Safi,
     /// Forwarding State (F) bit, which can be used to indicate whether the
     /// forwarding state for routes that were advertised with the given AFI
     /// and SAFI has indeed been preserved during the previous BGP restart.
@@ -905,11 +892,12 @@ mod tests {
     use eyre::Result;
 
     use crate::open::{
-        Capability, ExtendedNextHop, GracefulRestartEntry, MultiLabelEntry, OpenMessage, OpenOption,
+        AddPathSendRecv, Capability, ExtendedNextHop, GracefulRestartEntry, MultiLabelEntry,
+        OpenMessage, OpenOption,
     };
 
     use bgp_primitives::{
-        constants::{AddressFamilyId, SubsequentAfi},
+        constants::{AddressFamilyId, Afi, Safi},
         parser::ParserContext,
     };
 
@@ -941,8 +929,8 @@ mod tests {
         test_multiptotocol,
         &[0x01, 0x04, 0x00, 0x01, 0x00, 0x01],
         Capability::MultiProtocol {
-            afi: AddressFamilyId::Ipv4,
+            afi: Afi::IPV4,
-            safi: SubsequentAfi::Unicast
+            safi: Safi::UNICAST
         }
     );
 
@@ -971,8 +959,8 @@ mod tests {
             restart_state: false,
             restart_time: 300,
             entries: vec![GracefulRestartEntry {
-                afi: AddressFamilyId::Ipv4,
+                afi: Afi::IPV4,
-                safi: SubsequentAfi::Unicast,
+                safi: Safi::UNICAST,
                 preserve_forwarding: false,
             }]
         }
@@ -987,19 +975,19 @@ mod tests {
         Capability::ExtendedNextHop {
             entries: vec![
                 ExtendedNextHop {
-                    afi: AddressFamilyId::Ipv4,
+                    afi: Afi::IPV4,
-                    safi: SubsequentAfi::Unicast,
+                    safi: Safi::UNICAST,
-                    nh_afi: AddressFamilyId::Ipv6,
+                    nh_afi: Afi::IPV6,
                 },
                 ExtendedNextHop {
-                    afi: AddressFamilyId::Ipv4,
+                    afi: Afi::IPV4,
-                    safi: SubsequentAfi::Multicast,
+                    safi: Safi::MULTICAST,
-                    nh_afi: AddressFamilyId::Ipv6,
+                    nh_afi: Afi::IPV6,
                 },
                 ExtendedNextHop {
-                    afi: AddressFamilyId::Ipv4,
+                    afi: Afi::IPV4,
-                    safi: SubsequentAfi::MplsLabelledVpn,
+                    safi: Safi::MPLS_VPN,
-                    nh_afi: AddressFamilyId::Ipv6
+                    nh_afi: Afi::IPV6
                 }
             ]
         }
@@ -1021,8 +1009,8 @@ mod tests {
             identifier: Ipv4Addr::new(10, 0, 0, 1),
             options: vec![
                 OpenOption::Capabilities(vec![Capability::MultiProtocol {
-                    afi: AddressFamilyId::Ipv4,
+                    afi: Afi::IPV4,
-                    safi: SubsequentAfi::Unicast,
+                    safi: Safi::UNICAST,
                 }]),
                 OpenOption::Capabilities(vec![Capability::UnknownCapability {
                     type_code: 0x80,
@@ -1035,27 +1023,27 @@ mod tests {
                     restart_state: false,
                     restart_time: 300,
                     entries: vec![GracefulRestartEntry {
-                        afi: AddressFamilyId::Ipv4,
+                        afi: Afi::IPV4,
-                        safi: SubsequentAfi::Unicast,
+                        safi: Safi::UNICAST,
                         preserve_forwarding: false,
                     }],
                 }]),
                 OpenOption::Capabilities(vec![Capability::ExtendedNextHop {
                     entries: vec![
                         ExtendedNextHop {
-                            afi: AddressFamilyId::Ipv4,
+                            afi: Afi::IPV4,
-                            safi: SubsequentAfi::Unicast,
+                            safi: Safi::UNICAST,
-                            nh_afi: AddressFamilyId::Ipv6,
+                            nh_afi: Afi::IPV6,
                         },
                         ExtendedNextHop {
-                            afi: AddressFamilyId::Ipv4,
+                            afi: Afi::IPV4,
-                            safi: SubsequentAfi::Multicast,
+                            safi: Safi::MULTICAST,
-                            nh_afi: AddressFamilyId::Ipv6,
+                            nh_afi: Afi::IPV6,
                         },
                         ExtendedNextHop {
-                            afi: AddressFamilyId::Ipv4,
+                            afi: Afi::IPV4,
-                            safi: SubsequentAfi::MplsLabelledVpn,
+                            safi: Safi::MPLS_VPN,
-                            nh_afi: AddressFamilyId::Ipv6,
+                            nh_afi: Afi::IPV6,
                         },
                     ],
                 }]),
@@ -1088,13 +1076,13 @@ mod tests {
             Capability::MultiLabelCompat {
                 entries: vec![
                     MultiLabelEntry {
-                        afi: 1,
+                        afi: Afi::IPV4,
-                        safi: 4,
+                        safi: Safi::MPLS_LABEL,
                         count: 2
                     },
                     MultiLabelEntry {
-                        afi: 2,
+                        afi: Afi::IPV6,
-                        safi: 4,
+                        safi: Safi::MPLS_LABEL,
                         count: 3
                     },
                 ]
@@ -1108,37 +1096,51 @@ mod tests {
         assert!(Capability::from_wire(&ctx, &[0x08, 0x03, 0x00, 0x01, 0x04]).is_err());
     }
 
-    /// A peer may offer families we have no name for (here L2VPN EVPN,
+    /// A peer may offer families this implementation does not support
-    /// AFI 25 SAFI 70). That used to fail the whole OPEN. It is kept as an
+    /// (here L2VPN EVPN, AFI 25 SAFI 70). That used to fail the whole OPEN.
-    /// unknown capability, byte for byte, and what follows is still read.
+    /// The family is read as the numbers it is, and what follows it too.
     #[test]
-    fn a_capability_for_an_unknown_address_family_is_kept_as_unknown() {
+    fn a_capability_may_name_any_address_family() {
         let ctx = ParserContext::default();
-        let samples: &[(&str, &[u8])] = &[
+        let evpn = (Afi::L2VPN, Safi::EVPN);
-            ("multiprotocol", &[0x01, 0x04, 0x00, 0x19, 0x00, 0x46]),
+        let samples: &[(&[u8], Capability)] = &[
-            ("add-path", &[0x45, 0x04, 0x00, 0x19, 0x46, 0x03]),
+            (
+                &[0x01, 0x04, 0x00, 0x19, 0x00, 0x46],
+                Capability::MultiProtocol {
+                    afi: evpn.0,
+                    safi: evpn.1,
+                },
+            ),
+            (
+                &[0x45, 0x04, 0x00, 0x19, 0x46, 0x03],
+                Capability::AddPath {
+                    afi: evpn.0,
+                    safi: evpn.1,
+                    send_recv: AddPathSendRecv::Both,
+                },
+            ),
             (
-                "graceful restart",
                 &[0x40, 0x06, 0x00, 0x78, 0x00, 0x19, 0x46, 0x80],
+                Capability::GracefulRestart {
+                    restart_state: false,
+                    restart_time: 120,
+                    entries: vec![GracefulRestartEntry {
+                        afi: evpn.0,
+                        safi: evpn.1,
+                        preserve_forwarding: true,
+                    }],
+                },
             ),
         ];
-        for (name, wire) in samples {
+        for (wire, expected) in samples {
             let mut input = wire.to_vec();
             input.extend_from_slice(&[0x02, 0x00]);
             let (rest, cap) = Capability::from_wire(&ctx, &input).unwrap();
-            assert_eq!(rest, [0x02, 0x00], "{name}");
+            assert_eq!(rest, [0x02, 0x00], "{expected:?}");
-            assert_eq!(
+            assert_eq!(cap, *expected);
-                cap,
-                Capability::UnknownCapability {
-                    type_code: wire[0],
-                    length: wire[1],
-                    value: wire[2..].to_vec(),
-                },
-                "{name}"
-            );
             let mut out = BytesMut::new();
             cap.to_wire(&ctx, &mut out).unwrap();
-            assert_eq!(&out[..], *wire, "{name}");
+            assert_eq!(&out[..], *wire, "{expected:?}");
         }
 
         // A capability that is simply malformed is still an error.

crates/primitives/src/constants.rs

@@ -1,5 +1,6 @@
 use std::fmt::Display;
 
+use serde::{Deserialize, Serialize};
 use serde_repr::{Deserialize_repr, Serialize_repr};
 use thiserror::Error;
 
@@ -8,11 +9,8 @@ use thiserror::Error;
 #[error("unknown address family identifier {0}")]
 pub struct UnknownAfi(pub u16);
 
-/// A subsequent address family number this implementation has no name for.
+/// The IP version of a prefix. For the address family field of a capability
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Error)]
+/// or an MP_REACH_NLRI, which can name families that are not IP, see [`Afi`].
-#[error("unknown subsequent address family identifier {0}")]
-pub struct UnknownSafi(pub u16);
-
 #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, Serialize_repr, Deserialize_repr)]
 #[repr(u16)]
 pub enum AddressFamilyId {
@@ -41,51 +39,100 @@ impl Display for AddressFamilyId {
     }
 }
 
-/// Represents a Subsequent Address Family Identifier.
+/// An Address Family Identifier as it appears on the wire (RFC 4760).
-#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, Serialize_repr, Deserialize_repr)]
+///
-#[repr(u8)]
+/// Any number is a valid value: a peer may name a family this implementation
-pub enum SubsequentAfi {
+/// has never heard of, and that must be readable so it can be ignored. Which
-    Unicast = 1,
+/// families are *supported* is a separate question, answered where routes
-    Multicast = 2,
+/// are stored.
-    NlriWithMpls = 4,
+#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
-    MplsLabelledVpn = 128,
+#[serde(transparent)]
-    MulticastMplsVpn = 129,
+pub struct Afi(pub u16);
+
+impl Afi {
+    pub const IPV4: Self = Self(1);
+    pub const IPV6: Self = Self(2);
+    pub const L2VPN: Self = Self(25);
+    pub const BGP_LS: Self = Self(16388);
 }
 
-impl TryFrom<u8> for SubsequentAfi {
+impl From<AddressFamilyId> for Afi {
-    type Error = UnknownSafi;
+    fn from(version: AddressFamilyId) -> Self {
-
+        Self(version as u16)
-    fn try_from(value: u8) -> Result<Self, Self::Error> {
-        match value {
-            x if x == Self::Unicast as u8 => Ok(Self::Unicast),
-            x if x == Self::Multicast as u8 => Ok(Self::Multicast),
-            x if x == Self::NlriWithMpls as u8 => Ok(Self::NlriWithMpls),
-            x if x == Self::MplsLabelledVpn as u8 => Ok(Self::MplsLabelledVpn),
-            x if x == Self::MulticastMplsVpn as u8 => Ok(Self::MulticastMplsVpn),
-            other => Err(UnknownSafi(other.into())),
-        }
     }
 }
 
-/// Some encodings carry the SAFI in two octets. Every SAFI we know fits in
+impl TryFrom<Afi> for AddressFamilyId {
-/// one, so a larger value is unknown, not a smaller one in disguise.
+    type Error = UnknownAfi;
-impl TryFrom<u16> for SubsequentAfi {
-    type Error = UnknownSafi;
 
-    fn try_from(value: u16) -> Result<Self, Self::Error> {
+    fn try_from(afi: Afi) -> Result<Self, Self::Error> {
-        let narrow = u8::try_from(value).map_err(|_| UnknownSafi(value))?;
+        Self::try_from(afi.0)
-        Self::try_from(narrow)
     }
 }
 
-impl Display for SubsequentAfi {
+/// A Subsequent Address Family Identifier as it appears on the wire. Like
+/// [`Afi`], any number is a valid value.
+#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
+#[serde(transparent)]
+pub struct Safi(pub u8);
+
+impl Safi {
+    pub const UNICAST: Self = Self(1);
+    pub const MULTICAST: Self = Self(2);
+    pub const MPLS_LABEL: Self = Self(4);
+    pub const EVPN: Self = Self(70);
+    pub const BGP_LS: Self = Self(71);
+    pub const MPLS_VPN: Self = Self(128);
+    pub const MULTICAST_MPLS_VPN: Self = Self(129);
+    pub const FLOWSPEC: Self = Self(133);
+}
+
+/// An address family: the pair that says what kind of routes a capability
+/// or an MP_REACH_NLRI is about.
+#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
+pub struct AfiSafi {
+    pub afi: Afi,
+    pub safi: Safi,
+}
+
+impl AfiSafi {
+    pub const IPV4_UNICAST: Self = Self::new(Afi::IPV4, Safi::UNICAST);
+    pub const IPV6_UNICAST: Self = Self::new(Afi::IPV6, Safi::UNICAST);
+
+    pub const fn new(afi: Afi, safi: Safi) -> Self {
+        Self { afi, safi }
+    }
+
+    /// The unicast family of an IP version.
+    pub fn unicast(version: AddressFamilyId) -> Self {
+        Self::new(version.into(), Safi::UNICAST)
+    }
+
+    /// The usual name of the family, if it has one here.
+    fn name(self) -> Option<&'static str> {
+        Some(match (self.afi, self.safi) {
+            (Afi::IPV4, Safi::UNICAST) => "IPv4 unicast",
+            (Afi::IPV6, Safi::UNICAST) => "IPv6 unicast",
+            (Afi::IPV4, Safi::MULTICAST) => "IPv4 multicast",
+            (Afi::IPV6, Safi::MULTICAST) => "IPv6 multicast",
+            (Afi::IPV4, Safi::MPLS_LABEL) => "IPv4 labeled unicast",
+            (Afi::IPV6, Safi::MPLS_LABEL) => "IPv6 labeled unicast",
+            (Afi::IPV4, Safi::MPLS_VPN) => "VPNv4 unicast",
+            (Afi::IPV6, Safi::MPLS_VPN) => "VPNv6 unicast",
+            (Afi::IPV4, Safi::FLOWSPEC) => "IPv4 flowspec",
+            (Afi::IPV6, Safi::FLOWSPEC) => "IPv6 flowspec",
+            (Afi::L2VPN, Safi::EVPN) => "L2VPN EVPN",
+            (Afi::BGP_LS, Safi::BGP_LS) => "BGP-LS",
+            _ => return None,
+        })
+    }
+}
+
+impl Display for AfiSafi {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        match self {
+        match self.name() {
-            SubsequentAfi::Unicast => write!(f, "Unicast"),
+            Some(name) => f.write_str(name),
-            SubsequentAfi::Multicast => write!(f, "Multicast"),
+            None => write!(f, "AFI {} SAFI {}", self.afi.0, self.safi.0),
-            SubsequentAfi::NlriWithMpls => write!(f, "NlriWithMpls"),
-            SubsequentAfi::MplsLabelledVpn => write!(f, "MplsLabelledVpn"),
-            SubsequentAfi::MulticastMplsVpn => write!(f, "MulticastMplsVpn"),
         }
     }
 }
@@ -95,10 +142,29 @@ mod tests {
     use super::*;
 
     #[test]
-    fn a_wide_safi_is_not_truncated_into_a_known_one() {
+    fn any_family_is_a_value_and_the_common_ones_have_names() {
-        assert_eq!(SubsequentAfi::try_from(1u16), Ok(SubsequentAfi::Unicast));
+        assert_eq!(AfiSafi::IPV6_UNICAST.to_string(), "IPv6 unicast");
-        // 257 has the low byte of Unicast.
+        assert_eq!(
-        assert_eq!(SubsequentAfi::try_from(257u16), Err(UnknownSafi(257)));
+            AfiSafi::new(Afi::L2VPN, Safi::EVPN).to_string(),
+            "L2VPN EVPN"
+        );
+        assert_eq!(
+            AfiSafi::new(Afi(999), Safi(7)).to_string(),
+            "AFI 999 SAFI 7"
+        );
+        assert_eq!(
+            AfiSafi::unicast(AddressFamilyId::Ipv4),
+            AfiSafi::IPV4_UNICAST
+        );
+    }
+
+    #[test]
+    fn only_ipv4_and_ipv6_are_ip_versions() {
+        assert_eq!(
+            AddressFamilyId::try_from(Afi::IPV6),
+            Ok(AddressFamilyId::Ipv6)
+        );
+        assert_eq!(AddressFamilyId::try_from(Afi::L2VPN), Err(UnknownAfi(25)));
         assert_eq!(AddressFamilyId::try_from(3u16), Err(UnknownAfi(3)));
     }
 }

crates/primitives/src/parser.rs

@@ -57,8 +57,4 @@ pub enum ProtocolErrorValues {
     UnsupportedVersion(u8),
     /// `UnknownOpenOption` is returned with the open option type code that is unknown.
     UnknownOpenOption(u8),
-    /// A capability names an address family this implementation has no name
-    /// for. The capability parser turns this into an unknown capability; it
-    /// does not reach callers.
-    UnknownAddressFamily,
 }

crates/rib/src/entry.rs

@@ -110,7 +110,7 @@ mod tests {
     use bgp_packet::message::{
         MpReachNlriPathAttribute, MpUnreachNlriPathAttribute, OriginPathAttribute,
     };
-    use bgp_primitives::constants::{AddressFamilyId, SubsequentAfi};
+    use bgp_primitives::constants::{Afi, Safi};
 
     use super::*;
 
@@ -120,8 +120,8 @@ mod tests {
 
     fn mp_reach(prefixes: Vec<IpPrefix>) -> PathAttribute {
         PathAttribute::MpReachNlri(MpReachNlriPathAttribute {
-            afi: AddressFamilyId::Ipv6,
+            afi: Afi::IPV6,
-            safi: SubsequentAfi::Unicast,
+            safi: Safi::UNICAST,
             next_hop: NlriNextHop::Ipv6(Ipv6Addr::LOCALHOST),
             prefixes,
         })
@@ -167,8 +167,8 @@ mod tests {
                 PathAttribute::Origin(OriginPathAttribute::IGP),
                 mp_reach(vec![prefix("2001:db8:1::/48")]),
                 PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute {
-                    afi: AddressFamilyId::Ipv6,
+                    afi: Afi::IPV6,
-                    safi: SubsequentAfi::Unicast,
+                    safi: Safi::UNICAST,
                     prefixes: vec![prefix("2001:db8:2::/48")],
                 }),
             ],

crates/server/src/advertise.rs

@@ -14,7 +14,7 @@ use bgp_packet::message::{
     Message, MpReachNlriPathAttribute, MpUnreachNlriPathAttribute, MultiExitDiscPathAttribute,
     NextHopPathAttribute, NlriNextHop, PathAttribute, UpdateMessage,
 };
-use bgp_primitives::constants::{AddressFamilyId, SubsequentAfi};
+use bgp_primitives::constants::{AddressFamilyId, Afi, Safi};
 use bgp_primitives::ip_prefix::IpPrefix;
 use bgp_rib::{LocalRib, LocalRibEvent, LocalRoute};
 use tokio::sync::broadcast::error::RecvError;
@@ -188,8 +188,8 @@ pub fn build_announce(
     }
     if let (AddressFamilyId::Ipv6, IpAddr::V6(nh)) = (route.prefix.address_family, next_hop) {
         path_attributes.push(PathAttribute::MpReachNlri(MpReachNlriPathAttribute {
-            afi: AddressFamilyId::Ipv6,
+            afi: Afi::IPV6,
-            safi: SubsequentAfi::Unicast,
+            safi: Safi::UNICAST,
             next_hop: NlriNextHop::Ipv6(nh),
             prefixes: vec![route.prefix.clone()],
         }));
@@ -221,8 +221,8 @@ pub fn build_withdraw(prefix: &IpPrefix) -> UpdateMessage {
         AddressFamilyId::Ipv6 => UpdateMessage {
             withdrawn_routes: vec![],
             path_attributes: vec![PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute {
-                afi: AddressFamilyId::Ipv6,
+                afi: Afi::IPV6,
-                safi: SubsequentAfi::Unicast,
+                safi: Safi::UNICAST,
                 prefixes: vec![prefix.clone()],
             })],
             announced_routes: vec![],

crates/server/src/config.rs

@@ -4,11 +4,12 @@ use std::time::Duration;
 
 use bgp_packet::community;
 use bgp_packet::message::OriginPathAttribute;
+use bgp_primitives::constants::AfiSafi;
 use bgp_primitives::ip_prefix::IpPrefix;
 use bgp_rib::{LocalRoute, LocalRouteAttrs};
 
 use crate::error::{ConfigError, OriginateProblem, PeerProblem};
-use crate::family::{AddressFamily, FamilySet};
+use crate::family::FamilySet;
 use crate::proto::config::{self as proto, BgpConfig, OriginatedRoute, Peer};
 
 const DEFAULT_BGP_PORT: u16 = 179;
@@ -157,13 +158,13 @@ fn parse_peer(peer: Peer) -> Result<PeerConfig, PeerProblem> {
     };
 
     let families = if peer.address_families.is_empty() {
-        FamilySet::ALL
+        FamilySet::supported()
     } else {
         peer.address_families
             .iter()
             .map(|family| match family.enum_value() {
-                Ok(proto::AddressFamily::IPV4_UNICAST) => Ok(AddressFamily::Ipv4Unicast),
+                Ok(proto::AddressFamily::IPV4_UNICAST) => Ok(AfiSafi::IPV4_UNICAST),
-                Ok(proto::AddressFamily::IPV6_UNICAST) => Ok(AddressFamily::Ipv6Unicast),
+                Ok(proto::AddressFamily::IPV6_UNICAST) => Ok(AfiSafi::IPV6_UNICAST),
                 Ok(proto::AddressFamily::ADDRESS_FAMILY_UNSPECIFIED) | Err(_) => {
                     Err(PeerProblem::AddressFamily)
                 }
@@ -325,16 +326,16 @@ mod tests {
                 "{BASE}peers {{ name: \"r1\" endpoint {{ address: \"192.0.2.2\" }} remote_asn: 1 {body} }}"
             ))
             .unwrap();
-            config.peers[0].families
+            config.peers[0].families.clone()
         };
-        assert_eq!(families(""), FamilySet::ALL);
+        assert_eq!(families(""), FamilySet::supported());
         assert_eq!(
             families("address_families: IPV6_UNICAST"),
-            [AddressFamily::Ipv6Unicast].into_iter().collect()
+            [AfiSafi::IPV6_UNICAST].into_iter().collect()
         );
         assert_eq!(
             families("address_families: IPV4_UNICAST address_families: IPV6_UNICAST"),
-            FamilySet::ALL
+            FamilySet::supported()
         );
     }
 

crates/server/src/connection.rs

@@ -82,7 +82,7 @@ pub async fn run_session(
         local_asn: config.local_asn,
         hold_time_secs: config.hold_time_secs,
         expected_asn: config.expected_asn,
-        families: config.families,
+        families: config.families.clone(),
     });
     let mut session = Session {
         fsm,

crates/server/src/dialer.rs

@@ -37,7 +37,7 @@ pub fn spawn_dialers(
                 direction: Direction::Outbound,
                 peer_name: Some(peer.name.clone()),
                 expected_asn: Some(peer.remote_asn),
-                families: peer.families,
+                families: peer.families.clone(),
                 ..base.clone()
             };
             tokio::spawn(dial_loop(Arc::new(peer.clone()), config, connect_retry))

crates/server/src/family.rs

@@ -3,161 +3,94 @@
 //! Each side lists the families it is willing to exchange in its OPEN, as
 //! Multiprotocol capabilities (RFC 4760 §8). A family is in use on a session
 //! only if both sides listed it.
+//!
+//! Any family can be named, read from a peer and held in a [`FamilySet`].
+//! The ones this speaker can actually exchange routes for are [`SUPPORTED`],
+//! and only those are ever offered to a peer.
 
+use std::collections::BTreeSet;
 use std::fmt;
 
 use bgp_packet::message::{PathAttribute, UpdateMessage};
 use bgp_packet::open::{Capability, OpenMessage};
-use bgp_primitives::constants::{AddressFamilyId, SubsequentAfi};
+use bgp_primitives::constants::AfiSafi;
 use bgp_primitives::ip_prefix::IpPrefix;
 use bgp_primitives::parser::ParserContext;
 use bytes::BytesMut;
 
-/// An address family this speaker can exchange routes for.
+/// The address families this speaker can exchange routes for. Supporting a
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+/// new one means being able to parse, store and advertise its routes; then
-pub enum AddressFamily {
+/// it is added here.
-    Ipv4Unicast,
+pub const SUPPORTED: [AfiSafi; 2] = [AfiSafi::IPV4_UNICAST, AfiSafi::IPV6_UNICAST];
-    Ipv6Unicast,
-}
-
-impl AddressFamily {
-    pub const ALL: [Self; 2] = [Self::Ipv4Unicast, Self::Ipv6Unicast];
-
-    pub fn afi(self) -> AddressFamilyId {
-        match self {
-            Self::Ipv4Unicast => AddressFamilyId::Ipv4,
-            Self::Ipv6Unicast => AddressFamilyId::Ipv6,
-        }
-    }
-
-    pub fn safi(self) -> SubsequentAfi {
-        SubsequentAfi::Unicast
-    }
-
-    fn from_afi_safi(afi: AddressFamilyId, safi: SubsequentAfi) -> Option<Self> {
-        Self::ALL
-            .into_iter()
-            .find(|family| family.afi() == afi && family.safi() == safi)
-    }
-
-    /// The capability that offers this family to a peer.
-    pub fn capability(self) -> Capability {
-        Capability::MultiProtocol {
-            afi: self.afi(),
-            safi: self.safi(),
-        }
-    }
-
-    pub fn name(self) -> &'static str {
-        match self {
-            Self::Ipv4Unicast => "IPv4 unicast",
-            Self::Ipv6Unicast => "IPv6 unicast",
-        }
-    }
-}
-
-impl fmt::Display for AddressFamily {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        f.write_str(self.name())
-    }
-}
 
 /// A set of address families.
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
+#[derive(Debug, Clone, PartialEq, Eq, Default)]
-pub struct FamilySet {
+pub struct FamilySet(BTreeSet<AfiSafi>);
-    ipv4_unicast: bool,
-    ipv6_unicast: bool,
-}
 
 impl FamilySet {
-    pub const EMPTY: Self = Self {
+    /// Every family in [`SUPPORTED`].
-        ipv4_unicast: false,
+    pub fn supported() -> Self {
-        ipv6_unicast: false,
+        SUPPORTED.into_iter().collect()
-    };
-    pub const ALL: Self = Self {
-        ipv4_unicast: true,
-        ipv6_unicast: true,
-    };
-
-    pub fn insert(&mut self, family: AddressFamily) {
-        match family {
-            AddressFamily::Ipv4Unicast => self.ipv4_unicast = true,
-            AddressFamily::Ipv6Unicast => self.ipv6_unicast = true,
-        }
     }
 
-    pub fn contains(self, family: AddressFamily) -> bool {
+    pub fn insert(&mut self, family: AfiSafi) {
-        match family {
+        self.0.insert(family);
-            AddressFamily::Ipv4Unicast => self.ipv4_unicast,
-            AddressFamily::Ipv6Unicast => self.ipv6_unicast,
-        }
     }
 
-    fn contains_afi_safi(self, afi: AddressFamilyId, safi: SubsequentAfi) -> bool {
+    pub fn contains(&self, family: AfiSafi) -> bool {
-        AddressFamily::from_afi_safi(afi, safi).is_some_and(|family| self.contains(family))
+        self.0.contains(&family)
     }
 
     /// Whether a unicast route for `prefix` may be exchanged.
-    pub fn carries(self, prefix: &IpPrefix) -> bool {
+    pub fn carries(&self, prefix: &IpPrefix) -> bool {
-        self.contains_afi_safi(prefix.address_family, SubsequentAfi::Unicast)
+        self.contains(AfiSafi::unicast(prefix.address_family))
     }
 
-    pub fn is_empty(self) -> bool {
+    pub fn is_empty(&self) -> bool {
-        self == Self::EMPTY
+        self.0.is_empty()
     }
 
-    pub fn intersection(self, other: Self) -> Self {
+    pub fn intersection(&self, other: &Self) -> Self {
-        Self {
+        Self(self.0.intersection(&other.0).copied().collect())
-            ipv4_unicast: self.ipv4_unicast && other.ipv4_unicast,
-            ipv6_unicast: self.ipv6_unicast && other.ipv6_unicast,
-        }
     }
 
-    pub fn iter(self) -> impl Iterator<Item = AddressFamily> {
+    pub fn iter(&self) -> impl Iterator<Item = AfiSafi> + '_ {
-        AddressFamily::ALL
+        self.0.iter().copied()
-            .into_iter()
-            .filter(move |family| self.contains(*family))
     }
 
-    /// The families a peer offered in its OPEN.
+    /// The families a peer offered in its OPEN, whether or not we support
+    /// them.
     ///
     /// A peer that sends no Multiprotocol capability at all is a speaker of
     /// plain RFC 4271 BGP, which carries IPv4 unicast and nothing else. One
     /// that sends any has listed everything it wants, IPv4 unicast included.
     pub fn offered_by(open: &OpenMessage) -> Self {
-        let mut listed_any = false;
+        let listed: Self = open
-        let mut offered = Self::EMPTY;
+            .capabilities()
-        for capability in open.capabilities() {
+            .filter_map(|capability| match capability {
-            match capability {
+                Capability::MultiProtocol { afi, safi } => Some(AfiSafi::new(*afi, *safi)),
-                Capability::MultiProtocol { afi, safi } => {
+                _ => None,
-                    listed_any = true;
+            })
-                    if let Some(family) = AddressFamily::from_afi_safi(*afi, *safi) {
+            .collect();
-                        offered.insert(family);
+        if listed.is_empty() {
-                    }
+            [AfiSafi::IPV4_UNICAST].into_iter().collect()
-                }
-                // A Multiprotocol capability for a family we have no name
-                // for is kept as an unknown capability. It still shows that
-                // the peer lists its families.
-                Capability::UnknownCapability { type_code: 1, .. } => listed_any = true,
-                _ => {}
-            }
-        }
-        if listed_any {
-            offered
         } else {
-            [AddressFamily::Ipv4Unicast].into_iter().collect()
+            listed
         }
     }
 
     /// These families as Multiprotocol capabilities in wire form: the Data
     /// of an Unsupported Capability NOTIFICATION (RFC 5492 §5).
-    pub fn capabilities_wire(self) -> Vec<u8> {
+    pub fn capabilities_wire(&self) -> Vec<u8> {
         let ctx = ParserContext::default();
         let mut out = BytesMut::new();
         for family in self.iter() {
             // A Multiprotocol capability always encodes; if that ever
             // changed, a NOTIFICATION with less Data is still correct.
-            if family.capability().to_wire(&ctx, &mut out).is_err() {
+            if Capability::multiprotocol(family)
+                .to_wire(&ctx, &mut out)
+                .is_err()
+            {
                 return vec![];
             }
         }
@@ -170,20 +103,20 @@ impl FamilySet {
     /// The withdrawn routes and NLRI in the body of an UPDATE are IPv4
     /// unicast; every other family travels in MP_REACH_NLRI and
     /// MP_UNREACH_NLRI, which name theirs.
-    pub fn restrict(self, update: &mut UpdateMessage) -> usize {
+    pub fn restrict(&self, update: &mut UpdateMessage) -> usize {
         let mut removed = 0;
-        if !self.contains(AddressFamily::Ipv4Unicast) {
+        if !self.contains(AfiSafi::IPV4_UNICAST) {
             removed += update.withdrawn_routes.len() + update.announced_routes.len();
             update.withdrawn_routes.clear();
             update.announced_routes.clear();
         }
         update.path_attributes.retain(|attribute| {
-            let (afi, safi, prefixes) = match attribute {
+            let (family, prefixes) = match attribute {
-                PathAttribute::MpReachNlri(mp) => (mp.afi, mp.safi, mp.prefixes.len()),
+                PathAttribute::MpReachNlri(mp) => (mp.family(), mp.prefixes.len()),
-                PathAttribute::MpUnreachNlri(mp) => (mp.afi, mp.safi, mp.prefixes.len()),
+                PathAttribute::MpUnreachNlri(mp) => (mp.family(), mp.prefixes.len()),
                 _ => return true,
             };
-            let keep = self.contains_afi_safi(afi, safi);
+            let keep = self.contains(family);
             if !keep {
                 removed += prefixes;
             }
@@ -193,13 +126,9 @@ impl FamilySet {
     }
 }
 
-impl FromIterator<AddressFamily> for FamilySet {
+impl FromIterator<AfiSafi> for FamilySet {
-    fn from_iter<I: IntoIterator<Item = AddressFamily>>(families: I) -> Self {
+    fn from_iter<I: IntoIterator<Item = AfiSafi>>(families: I) -> Self {
-        let mut set = Self::EMPTY;
+        Self(families.into_iter().collect())
-        for family in families {
-            set.insert(family);
-        }
-        set
     }
 }
 
@@ -212,7 +141,7 @@ impl fmt::Display for FamilySet {
             if i > 0 {
                 f.write_str(", ")?;
             }
-            f.write_str(family.name())?;
+            write!(f, "{family}")?;
         }
         Ok(())
     }
@@ -226,6 +155,7 @@ mod tests {
         MpReachNlriPathAttribute, MpUnreachNlriPathAttribute, NlriNextHop, OriginPathAttribute,
     };
     use bgp_packet::open::OpenOption;
+    use bgp_primitives::constants::{Afi, Safi};
 
     use super::*;
 
@@ -247,7 +177,7 @@ mod tests {
         IpPrefix::try_from(s).unwrap()
     }
 
-    fn only(family: AddressFamily) -> FamilySet {
+    fn only(family: AfiSafi) -> FamilySet {
         [family].into_iter().collect()
     }
 
@@ -255,59 +185,68 @@ mod tests {
     fn a_peer_without_multiprotocol_capabilities_speaks_ipv4_unicast() {
         assert_eq!(
             FamilySet::offered_by(&open(vec![])),
-            only(AddressFamily::Ipv4Unicast)
+            only(AfiSafi::IPV4_UNICAST)
         );
         // Other capabilities do not change that.
         assert_eq!(
             FamilySet::offered_by(&open(vec![Capability::RouteRefresh])),
-            only(AddressFamily::Ipv4Unicast)
+            only(AfiSafi::IPV4_UNICAST)
         );
     }
 
     #[test]
     fn a_peer_that_lists_families_gets_exactly_those() {
         assert_eq!(
-            FamilySet::offered_by(&open(vec![AddressFamily::Ipv6Unicast.capability()])),
+            FamilySet::offered_by(&open(vec![Capability::multiprotocol(
-            only(AddressFamily::Ipv6Unicast),
+                AfiSafi::IPV6_UNICAST
+            )])),
+            only(AfiSafi::IPV6_UNICAST),
             "IPv4 unicast is not implied once a peer lists its families"
         );
         // Capabilities may be spread over several optional parameters.
-        let mut spread = open(vec![AddressFamily::Ipv4Unicast.capability()]);
+        let mut spread = open(vec![Capability::multiprotocol(AfiSafi::IPV4_UNICAST)]);
-        spread.options.push(OpenOption::Capabilities(vec![
+        spread
-            AddressFamily::Ipv6Unicast.capability(),
+            .options
+            .push(OpenOption::Capabilities(vec![Capability::multiprotocol(
+                AfiSafi::IPV6_UNICAST,
+            )]));
+        assert_eq!(FamilySet::offered_by(&spread), FamilySet::supported());
+        // A family we do not support is still what the peer offered. It
+        // drops out when intersected with what we offer.
+        let evpn = AfiSafi::new(Afi::L2VPN, Safi::EVPN);
+        let offered = FamilySet::offered_by(&open(vec![
+            Capability::multiprotocol(evpn),
+            Capability::multiprotocol(AfiSafi::IPV6_UNICAST),
         ]));
-        assert_eq!(FamilySet::offered_by(&spread), FamilySet::ALL);
+        assert_eq!(offered.to_string(), "IPv6 unicast, L2VPN EVPN");
-        // IPv6 multicast is a family, but not one we carry.
         assert_eq!(
-            FamilySet::offered_by(&open(vec![Capability::MultiProtocol {
+            FamilySet::supported().intersection(&offered),
-                afi: AddressFamilyId::Ipv6,
+            only(AfiSafi::IPV6_UNICAST)
-                safi: SubsequentAfi::Multicast,
-            }])),
-            FamilySet::EMPTY
         );
-        // Nor is one we cannot even name (here L2VPN EVPN, 25/70).
+        assert!(
-        assert_eq!(
+            FamilySet::supported()
-            FamilySet::offered_by(&open(vec![Capability::UnknownCapability {
+                .intersection(&FamilySet::offered_by(&open(vec![
-                type_code: 1,
+                    Capability::multiprotocol(evpn)
-                length: 4,
+                ])))
-                value: vec![0, 25, 0, 70],
+                .is_empty()
-            }])),
-            FamilySet::EMPTY
         );
     }
 
     #[test]
     fn sets_intersect_and_describe_themselves() {
-        let v6 = only(AddressFamily::Ipv6Unicast);
+        let v6 = only(AfiSafi::IPV6_UNICAST);
-        assert_eq!(FamilySet::ALL.intersection(v6), v6);
+        assert_eq!(FamilySet::supported().intersection(&v6), v6);
-        assert!(only(AddressFamily::Ipv4Unicast).intersection(v6).is_empty());
+        assert!(only(AfiSafi::IPV4_UNICAST).intersection(&v6).is_empty());
-        assert_eq!(FamilySet::ALL.to_string(), "IPv4 unicast, IPv6 unicast");
+        assert_eq!(
-        assert_eq!(FamilySet::EMPTY.to_string(), "none");
+            FamilySet::supported().to_string(),
+            "IPv4 unicast, IPv6 unicast"
+        );
+        assert_eq!(FamilySet::default().to_string(), "none");
         assert!(v6.carries(&prefix("2001:db8::/32")));
         assert!(!v6.carries(&prefix("192.0.2.0/24")));
         // One capability each: code 1, length 4, AFI, reserved, SAFI.
         assert_eq!(
-            FamilySet::ALL.capabilities_wire(),
+            FamilySet::supported().capabilities_wire(),
             [1, 4, 0, 1, 0, 1, 1, 4, 0, 2, 0, 1]
         );
     }
@@ -318,14 +257,14 @@ mod tests {
             path_attributes: vec![
                 PathAttribute::Origin(OriginPathAttribute::IGP),
                 PathAttribute::MpReachNlri(MpReachNlriPathAttribute {
-                    afi: AddressFamilyId::Ipv6,
+                    afi: Afi::IPV6,
-                    safi: SubsequentAfi::Unicast,
+                    safi: Safi::UNICAST,
                     next_hop: NlriNextHop::Ipv6(Ipv6Addr::LOCALHOST),
                     prefixes: vec![prefix("2001:db8:1::/48"), prefix("2001:db8:2::/48")],
                 }),
                 PathAttribute::MpUnreachNlri(MpUnreachNlriPathAttribute {
-                    afi: AddressFamilyId::Ipv6,
+                    afi: Afi::IPV6,
-                    safi: SubsequentAfi::Unicast,
+                    safi: Safi::UNICAST,
                     prefixes: vec![prefix("2001:db8:3::/48")],
                 }),
             ],
@@ -336,11 +275,11 @@ mod tests {
     #[test]
     fn restrict_removes_the_routes_of_families_not_in_the_set() {
         let mut update = mixed_update();
-        assert_eq!(FamilySet::ALL.restrict(&mut update), 0);
+        assert_eq!(FamilySet::supported().restrict(&mut update), 0);
         assert_eq!(update, mixed_update());
 
         let mut update = mixed_update();
-        assert_eq!(only(AddressFamily::Ipv4Unicast).restrict(&mut update), 3);
+        assert_eq!(only(AfiSafi::IPV4_UNICAST).restrict(&mut update), 3);
         assert_eq!(update.announced_routes, vec![prefix("192.0.2.0/24")]);
         assert_eq!(update.withdrawn_routes, vec![prefix("198.51.100.0/24")]);
         assert_eq!(
@@ -349,7 +288,7 @@ mod tests {
         );
 
         let mut update = mixed_update();
-        assert_eq!(only(AddressFamily::Ipv6Unicast).restrict(&mut update), 2);
+        assert_eq!(only(AfiSafi::IPV6_UNICAST).restrict(&mut update), 2);
         assert!(update.announced_routes.is_empty());
         assert!(update.withdrawn_routes.is_empty());
         assert_eq!(update.path_attributes.len(), 3);

crates/server/src/fsm.rs

@@ -65,7 +65,7 @@ pub struct FsmConfig {
 }
 
 /// What the two OPENs settled.
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub struct Negotiated {
     pub peer_asn: u32,
     pub peer_identifier: Ipv4Addr,
@@ -124,7 +124,7 @@ pub enum Action {
 /// [`State`] together with what is known in it. What the OPENs settled
 /// exists exactly when the handshake has got that far, so it lives in the
 /// states that have it and nowhere else.
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug, Clone)]
 enum Phase {
     OpenSent,
     OpenConfirm(Negotiated),
@@ -164,8 +164,8 @@ impl Fsm {
     }
 
     /// What the OPENs settled; `None` until the peer's OPEN is accepted.
-    pub fn negotiated(&self) -> Option<Negotiated> {
+    pub fn negotiated(&self) -> Option<&Negotiated> {
-        match self.phase {
+        match &self.phase {
             Phase::OpenConfirm(negotiated) | Phase::Established(negotiated) => Some(negotiated),
             Phase::OpenSent | Phase::Closed => None,
         }
@@ -193,7 +193,7 @@ impl Fsm {
     }
 
     fn on_message(&mut self, msg: Message) -> Vec<Action> {
-        match (self.phase, msg) {
+        match (&self.phase, msg) {
             // A NOTIFICATION ends the session in any state, and is not
             // answered (RFC 4271 §8.2.2).
             (_, Message::Notification(notification)) => {
@@ -201,9 +201,11 @@ impl Fsm {
             }
             (Phase::OpenSent, Message::Open(open)) => self.on_open(open),
             (Phase::OpenConfirm(negotiated), Message::KeepAlive) => {
-                self.phase = Phase::Established(negotiated);
+                let negotiated = negotiated.clone();
+                let hold = hold_time(&negotiated);
+                self.phase = Phase::Established(negotiated.clone());
                 let mut actions = vec![Action::Established(negotiated)];
-                match hold_time(negotiated) {
+                match hold {
                     Some(hold) => {
                         actions.push(Action::ResetHoldTimer(hold));
                         actions.push(Action::StartKeepaliveTimer(hold / 3));
@@ -257,9 +259,9 @@ impl Fsm {
             });
         }
 
-        let ours = self.config.families;
+        let ours = self.config.families.clone();
         let theirs = FamilySet::offered_by(&open);
-        let families = ours.intersection(theirs);
+        let families = ours.intersection(&theirs);
         if families.is_empty() {
             return self.fail(SessionError::NoCommonAddressFamily { ours, theirs });
         }
@@ -271,14 +273,14 @@ impl Fsm {
             four_octet_asn: capability_asn.is_some(),
             families,
         };
-        self.phase = Phase::OpenConfirm(negotiated);
+        // With a hold time of zero the timer will not run once Established,
+        // but a peer that never confirms must still be given up on.
+        let confirm_within = hold_time(&negotiated).unwrap_or(INITIAL_HOLD_TIME);
+        self.phase = Phase::OpenConfirm(negotiated.clone());
         vec![
             Action::Admit(negotiated),
             Action::Send(Message::KeepAlive),
-            // With a hold time of zero the timer will not run once
+            Action::ResetHoldTimer(confirm_within),
-            // Established, but a peer that never confirms must still be
-            // given up on.
-            Action::ResetHoldTimer(hold_time(negotiated).unwrap_or(INITIAL_HOLD_TIME)),
         ]
     }
 
@@ -315,7 +317,7 @@ impl Fsm {
 }
 
 /// Any message from the peer shows it is alive (RFC 4271 §4.4).
-fn heard_from_peer(negotiated: Negotiated) -> Vec<Action> {
+fn heard_from_peer(negotiated: &Negotiated) -> Vec<Action> {
     hold_time(negotiated)
         .map(Action::ResetHoldTimer)
         .into_iter()
@@ -323,7 +325,7 @@ fn heard_from_peer(negotiated: Negotiated) -> Vec<Action> {
 }
 
 /// The hold time as a duration; `None` when the timers do not run.
-fn hold_time(negotiated: Negotiated) -> Option<Duration> {
+fn hold_time(negotiated: &Negotiated) -> Option<Duration> {
     match negotiated.hold_time_secs {
         0 => None,
         secs => Some(Duration::from_secs(secs.into())),
@@ -339,7 +341,7 @@ fn build_open(config: &FsmConfig) -> Message {
     }];
     // One Multiprotocol capability per family we will exchange. A peer does
     // not accept routes of a family that was not offered (RFC 4760 §8).
-    capabilities.extend(config.families.iter().map(|family| family.capability()));
+    capabilities.extend(config.families.iter().map(Capability::multiprotocol));
     capabilities.push(Capability::RouteRefresh);
     Message::Open(OpenMessage {
         version: BGP4_VERSION,
@@ -371,11 +373,11 @@ mod tests {
     use bgp_packet::message::{
         MpReachNlriPathAttribute, NlriNextHop, NotificationMessage, PathAttribute,
     };
-    use bgp_primitives::constants::{AddressFamilyId, SubsequentAfi};
+    use bgp_primitives::constants::{Afi, AfiSafi, Safi};
     use bgp_primitives::ip_prefix::IpPrefix;
 
     use super::*;
-    use crate::family::AddressFamily;
+    use crate::family::SUPPORTED;
 
     const LOCAL_ID: Ipv4Addr = Ipv4Addr::new(10, 0, 0, 1);
     const PEER_ID: Ipv4Addr = Ipv4Addr::new(10, 0, 0, 2);
@@ -386,11 +388,11 @@ mod tests {
             local_asn: 65000,
             hold_time_secs: 90,
             expected_asn: None,
-            families: FamilySet::ALL,
+            families: FamilySet::supported(),
         }
     }
 
-    fn only(family: AddressFamily) -> FamilySet {
+    fn only(family: AfiSafi) -> FamilySet {
         [family].into_iter().collect()
     }
 
@@ -478,7 +480,7 @@ mod tests {
             hold_time_secs: 30,
             four_octet_asn: false,
             // The peer listed no families, so it speaks IPv4 unicast.
-            families: only(AddressFamily::Ipv4Unicast),
+            families: only(AfiSafi::IPV4_UNICAST),
         };
         assert!(matches!(
             &actions[..],
@@ -544,7 +546,7 @@ mod tests {
     #[test]
     fn our_open_offers_exactly_the_configured_families() {
         let (_, actions) = Fsm::new(FsmConfig {
-            families: only(AddressFamily::Ipv6Unicast),
+            families: only(AfiSafi::IPV6_UNICAST),
             ..config()
         });
         let Action::Send(open) = &actions[0] else {
@@ -554,7 +556,7 @@ mod tests {
             capabilities(open),
             [
                 &Capability::FourByteAsn { asn: 65000 },
-                &AddressFamily::Ipv6Unicast.capability(),
+                &Capability::multiprotocol(AfiSafi::IPV6_UNICAST),
                 &Capability::RouteRefresh,
             ]
         );
@@ -562,14 +564,14 @@ mod tests {
 
     #[test]
     fn the_session_carries_the_families_both_sides_offered() {
-        let negotiated = |ours, theirs: Vec<AddressFamily>| {
+        let negotiated = |ours, theirs: Vec<AfiSafi>| {
             let (mut fsm, _) = Fsm::new(FsmConfig {
                 families: ours,
                 ..config()
             });
             let open = OpenMessage {
                 options: vec![OpenOption::Capabilities(
-                    theirs.into_iter().map(AddressFamily::capability).collect(),
+                    theirs.into_iter().map(Capability::multiprotocol).collect(),
                 )],
                 ..peer_open(90)
             };
@@ -577,26 +579,20 @@ mod tests {
             (fsm, actions)
         };
 
-        let (fsm, _) = negotiated(FamilySet::ALL, vec![AddressFamily::Ipv6Unicast]);
+        let (fsm, _) = negotiated(FamilySet::supported(), vec![AfiSafi::IPV6_UNICAST]);
         assert_eq!(
             fsm.negotiated().unwrap().families,
-            only(AddressFamily::Ipv6Unicast)
+            only(AfiSafi::IPV6_UNICAST)
-        );
-        let (fsm, _) = negotiated(
-            only(AddressFamily::Ipv4Unicast),
-            AddressFamily::ALL.to_vec(),
         );
+        let (fsm, _) = negotiated(only(AfiSafi::IPV4_UNICAST), SUPPORTED.to_vec());
         assert_eq!(
             fsm.negotiated().unwrap().families,
-            only(AddressFamily::Ipv4Unicast)
+            only(AfiSafi::IPV4_UNICAST)
         );
 
         // Nothing in common: the session could carry no routes at all. The
         // NOTIFICATION names the capability we needed.
-        let (fsm, actions) = negotiated(
+        let (fsm, actions) = negotiated(only(AfiSafi::IPV4_UNICAST), vec![AfiSafi::IPV6_UNICAST]);
-            only(AddressFamily::Ipv4Unicast),
-            vec![AddressFamily::Ipv6Unicast],
-        );
         assert!(matches!(
             &actions[0],
             Action::Send(Message::Notification(n))
@@ -617,8 +613,8 @@ mod tests {
         let update = UpdateMessage {
             withdrawn_routes: vec![],
             path_attributes: vec![PathAttribute::MpReachNlri(MpReachNlriPathAttribute {
-                afi: AddressFamilyId::Ipv6,
+                afi: Afi::IPV6,
-                safi: SubsequentAfi::Unicast,
+                safi: Safi::UNICAST,
                 next_hop: NlriNextHop::Ipv6(std::net::Ipv6Addr::LOCALHOST),
                 prefixes: vec![IpPrefix::try_from("2001:db8::/32").unwrap()],
             })],

crates/server/src/server.rs

@@ -116,7 +116,7 @@ impl BgpServer {
             direction: Direction::Inbound,
             peer_name: None,
             expected_asn: None,
-            families: FamilySet::ALL,
+            families: FamilySet::supported(),
         };
 
         let _advertiser = spawn_advertiser(
@@ -151,7 +151,7 @@ impl BgpServer {
                         config.peer_name = Some(peer.name.clone());
                         config.expected_asn = Some(peer.remote_asn);
                         config.hold_time_secs = peer.hold_time_secs;
-                        config.families = peer.families;
+                        config.families = peer.families.clone();
                     }
                     tokio::spawn(handle_connection(stream, peer_addr, Arc::new(config)));
                 }