address review comments

Signed-off-by: fliqqs <[email protected]>
holo-routing · Jan 10, 2025 · fdadb83 · fdadb83
1 parent 3e209b6
commit fdadb83
Show file tree

Hide file tree

Showing 7 changed files with 46 additions and 75 deletions.
diff --git a/holo-ospf/src/instance.rs b/holo-ospf/src/instance.rs
@@ -103,7 +103,7 @@ pub struct InstanceState<V: Version> {
     pub gr_helper_count: usize,
     // Authentication non-decreasing sequence number.
     pub auth_seqno: Arc<AtomicU64>,
-    // Hostname cache
+    // Hostname cache.
     pub hostnames: BTreeMap<Ipv4Addr, String>,
 }
 

diff --git a/holo-ospf/src/ospfv2/lsdb.rs b/holo-ospf/src/ospfv2/lsdb.rs
@@ -13,7 +13,6 @@ use holo_utils::mpls::Label;
 use holo_utils::sr::{IgpAlgoType, Sid, SidLastHopBehavior};
 use ipnetwork::{IpNetwork, Ipv4Network};
 use itertools::Itertools;
-use tracing::debug;
 
 use crate::area::{Area, AreaType, AreaVersion, OptionsLocation};
 use crate::collections::{
@@ -354,41 +353,19 @@ impl LsdbVersion<Self> for Ospfv2 {
             }
         }
 
-        // examine for DynamicHostnameTlv
-        if lsa.hdr.lsa_type.type_code() == Some(LsaTypeCode::OpaqueArea) {
-            if let LsaBody::OpaqueArea(LsaOpaque::RouterInfo(router_info)) =
-                &lsa.body
-            {
-                if let Some(hostname_tlv) = router_info.info_hostname.as_ref() {
-                    // install or update hostname
-                    instance
-                        .state
-                        .hostnames
-                        .insert(lsa.hdr.adv_rtr, hostname_tlv.hostname.clone());
-                } else {
-                    // remove hostname if it exists
-                    instance.state.hostnames.remove(&lsa.hdr.adv_rtr);
-                }
-            }
-        }
-        if lsa.hdr.lsa_type.type_code() == Some(LsaTypeCode::OpaqueAs) {
-            if let LsaBody::OpaqueAs(LsaOpaque::RouterInfo(router_info)) =
-                &lsa.body
-            {
-                if let Some(hostname_tlv) = router_info.info_hostname.as_ref() {
-                    debug!(
-                        "Router {} has hostname {}",
-                        lsa.hdr.adv_rtr, hostname_tlv.hostname
-                    );
-                    // install or update hostname
-                    instance
-                        .state
-                        .hostnames
-                        .insert(lsa.hdr.adv_rtr, hostname_tlv.hostname.clone());
-                } else {
-                    // remove hostname if it exists
-                    instance.state.hostnames.remove(&lsa.hdr.adv_rtr);
-                }
+        // Update hostname database.
+        if let LsaBody::OpaqueArea(LsaOpaque::RouterInfo(router_info))
+        | LsaBody::OpaqueAs(LsaOpaque::RouterInfo(router_info)) = &lsa.body
+        {
+            if let Some(hostname_tlv) = router_info.info_hostname.as_ref() {
+                // Install or update hostname.
+                instance
+                    .state
+                    .hostnames
+                    .insert(lsa.hdr.adv_rtr, hostname_tlv.hostname.clone());
+            } else {
+                // Remove hostname if it exists.
+                instance.state.hostnames.remove(&lsa.hdr.adv_rtr);
             }
         }
     }

diff --git a/holo-ospf/src/ospfv2/packet/lsa_opaque.rs b/holo-ospf/src/ospfv2/packet/lsa_opaque.rs
@@ -122,7 +122,7 @@ pub struct LsaRouterInfo {
     pub srlb: Vec<SrLocalBlockTlv>,
     pub msds: Option<MsdTlv>,
     pub srms_pref: Option<SrmsPrefTlv>,
-    // #[serde(skip)]
+    #[serde(skip)]
     pub info_hostname: Option<DynamicHostnameTlv>,
     pub unknown_tlvs: Vec<UnknownTlv>,
 }

diff --git a/holo-ospf/src/ospfv3/lsdb.rs b/holo-ospf/src/ospfv3/lsdb.rs
@@ -14,7 +14,6 @@ use holo_utils::mpls::Label;
 use holo_utils::sr::{IgpAlgoType, Sid, SidLastHopBehavior};
 use ipnetwork::IpNetwork;
 use itertools::Itertools;
-use tracing::debug;
 
 use crate::area::{Area, AreaType, AreaVersion, OptionsLocation};
 use crate::collections::{
@@ -283,9 +282,9 @@ impl LsdbVersion<Self> for Ospfv3 {
                 }
             }
             LsaOriginateEvent::HostnameChange => {
-                // (Re)originate Router-LSA(s) in all areas.
+                // (Re)originate Router-Info-LSA(s) in all areas.
                 for area in arenas.areas.iter() {
-                    lsa_orig_router(area, instance, arenas);
+                    lsa_orig_router_info(area, instance);
                 }
             }
             LsaOriginateEvent::BierEnableChange => {
@@ -448,10 +447,6 @@ impl LsdbVersion<Self> for Ospfv3 {
         {
             if let LsaBody::RouterInfo(router_info) = &lsa.body {
                 if let Some(hostname_tlv) = router_info.info_hostname.as_ref() {
-                    debug!(
-                        "Router {} has hostname {}",
-                        lsa.hdr.adv_rtr, hostname_tlv.hostname
-                    );
                     instance
                         .state
                         .hostnames

diff --git a/holo-ospf/src/packet/tlv.rs b/holo-ospf/src/packet/tlv.rs
@@ -82,8 +82,6 @@ bitflags! {
 #[derive(Deserialize, Serialize)]
 pub struct RouterInfoCapsTlv(RouterInfoCaps);
 
-const MAX_HOSTNAME_LEN: usize = 255;
-
 // OSPF Router Functional Capability Bits.
 //
 // IANA registry:
@@ -132,6 +130,8 @@ pub struct DynamicHostnameTlv {
     pub hostname: String,
 }
 
+const MAX_HOSTNAME_LEN: usize = 255;
+
 //
 // SR-Algorithm TLV.
 //
@@ -560,30 +560,29 @@ impl From<RouterFuncCaps> for RouterFuncCapsTlv {
 
 impl DynamicHostnameTlv {
     pub(crate) fn decode(tlv_len: u16, buf: &mut Bytes) -> DecodeResult<Self> {
-        let mut hostname = String::new();
-        for _ in 0..tlv_len {
-            let c = buf.get_u8();
-            if c == 0 {
-                break;
-            }
-            hostname.push(c as char);
+        if tlv_len == 0 {
+            return Err(DecodeError::InvalidTlvLength(tlv_len));
         }
 
+        let mut hostname_bytes = [0; 255];
+        buf.copy_to_slice(
+            &mut hostname_bytes
+                [..usize::min(tlv_len as usize, MAX_HOSTNAME_LEN)],
+        );
+
+        let hostname =
+            String::from_utf8_lossy(&hostname_bytes[..tlv_len as usize])
+                .to_string();
+
         Ok(DynamicHostnameTlv { hostname })
     }
 
     pub(crate) fn encode(&self, buf: &mut BytesMut) {
         let start_pos =
             tlv_encode_start(buf, RouterInfoTlvType::DynamicHostname);
-        let mut hostname = self.hostname.clone();
-
-        if hostname.len() > MAX_HOSTNAME_LEN {
-            hostname.truncate(MAX_HOSTNAME_LEN);
-        }
-
-        for c in hostname.chars() {
-            buf.put_u8(c as u8);
-        }
+        let hostname = self.hostname.as_bytes();
+        let hostname_len = usize::min(hostname.len(), MAX_HOSTNAME_LEN);
+        buf.put_slice(&hostname[..hostname_len]);
         tlv_encode_end(buf, start_pos);
     }
 

diff --git a/holo-ospf/tests/packet/ospfv2.rs b/holo-ospf/tests/packet/ospfv2.rs
@@ -570,12 +570,12 @@ static LSA2: Lazy<(Vec<u8>, Lsa<Ospfv2>)> = Lazy::new(|| {
     (
         vec![
             0x00, 0x01, 0x42, 0x0a, 0x04, 0x00, 0x00, 0x00, 0x01, 0x01, 0x01,
-            0x01, 0x80, 0x00, 0x00, 0x01, 0x20, 0x95, 0x00, 0x44, 0x00, 0x01,
-            0x00, 0x04, 0x10, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x01, 0x00,
-            0x00, 0x00, 0x00, 0x00, 0x09, 0x00, 0x0b, 0x00, 0x1f, 0x40, 0x00,
-            0x00, 0x01, 0x00, 0x03, 0x00, 0x3e, 0x80, 0x00, 0x00, 0x0e, 0x00,
-            0x0b, 0x00, 0x03, 0xe8, 0x00, 0x00, 0x01, 0x00, 0x03, 0x00, 0x3a,
-            0x98, 0x00,
+            0x01, 0x80, 0x00, 0x00, 0x01, 0xb3, 0x3b, 0x00, 0x4c, 0x00, 0x01,
+            0x00, 0x04, 0x10, 0x00, 0x00, 0x00, 0x00, 0x07, 0x00, 0x04, 0x68,
+            0x6f, 0x6c, 0x6f, 0x00, 0x08, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00,
+            0x00, 0x09, 0x00, 0x0b, 0x00, 0x1f, 0x40, 0x00, 0x00, 0x01, 0x00,
+            0x03, 0x00, 0x3e, 0x80, 0x00, 0x00, 0x0e, 0x00, 0x0b, 0x00, 0x03,
+            0xe8, 0x00, 0x00, 0x01, 0x00, 0x03, 0x00, 0x3a, 0x98, 0x00,
         ],
         Lsa::new(
             1,

diff --git a/holo-ospf/tests/packet/ospfv3.rs b/holo-ospf/tests/packet/ospfv3.rs
@@ -624,12 +624,12 @@ static LSA3: Lazy<(Vec<u8>, Lsa<Ospfv3>)> = Lazy::new(|| {
     (
         vec![
             0x00, 0x01, 0xa0, 0x0c, 0x00, 0x00, 0x00, 0x00, 0x01, 0x01, 0x01,
-            0x01, 0x80, 0x00, 0x00, 0x01, 0xf5, 0xa2, 0x00, 0x44, 0x00, 0x01,
-            0x00, 0x04, 0xd0, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00, 0x01, 0x00,
-            0x00, 0x00, 0x00, 0x00, 0x09, 0x00, 0x0b, 0x00, 0x1f, 0x40, 0x00,
-            0x00, 0x01, 0x00, 0x03, 0x00, 0x3e, 0x80, 0x00, 0x00, 0x0e, 0x00,
-            0x0b, 0x00, 0x03, 0xe8, 0x00, 0x00, 0x01, 0x00, 0x03, 0x00, 0x3a,
-            0x98, 0x00,
+            0x01, 0x80, 0x00, 0x00, 0x01, 0x89, 0x48, 0x00, 0x4c, 0x00, 0x01,
+            0x00, 0x04, 0xd0, 0x00, 0x00, 0x00, 0x00, 0x07, 0x00, 0x04, 0x68,
+            0x6f, 0x6c, 0x6f, 0x00, 0x08, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00,
+            0x00, 0x09, 0x00, 0x0b, 0x00, 0x1f, 0x40, 0x00, 0x00, 0x01, 0x00,
+            0x03, 0x00, 0x3e, 0x80, 0x00, 0x00, 0x0e, 0x00, 0x0b, 0x00, 0x03,
+            0xe8, 0x00, 0x00, 0x01, 0x00, 0x03, 0x00, 0x3a, 0x98, 0x00,
         ],
         Lsa::new(
             1,