chore(msg-sim): address review

Author: thedevbirb

.github/workflows/ci.yml

@@ -53,8 +53,12 @@ jobs:
         with:
           cache-on-failure: true
 
+      # Install nextest
+      - name: Install cargo-nextest
+        uses: taiki-e/install-action@nextest
+
       - name: Run msg-sim tests (requires root for network namespaces)
-        run: sudo env "PATH=$PATH" "HOME=$HOME" cargo test run -p msg-sim --all-features --no-fail-fast -- --test-threads=1
+        run: sudo env "PATH=$PATH" "HOME=$HOME" cargo nextest run -p msg-sim --all-features --no-fail-fast --test-threads=1
 
   cargo-lint:
     runs-on: ubuntu-latest

Cargo.lock

@@ -19,12 +19,16 @@
 //! sudo HOME=$HOME RUST_LOG=info $(which cargo) run --example multi_region -p msg-sim
 //! ```
 
-use std::net::{IpAddr, Ipv4Addr, SocketAddr};
-use std::time::{Duration, Instant};
-
-use msg_sim::ip::Subnet;
-use msg_sim::network::{Link, Network, PeerIdExt};
-use msg_sim::tc::impairment::LinkImpairment;
+use std::{
+    net::{IpAddr, Ipv4Addr, SocketAddr},
+    time::{Duration, Instant},
+};
+
+use msg_sim::{
+    ip::Subnet,
+    network::{Link, Network, PeerIdExt},
+    tc::impairment::LinkImpairment,
+};
 use tracing_subscriber::EnvFilter;
 
 // Peer IDs (assigned sequentially by add_peer)

msg-sim/examples/sim_multi_region.rs

@@ -1,8 +1,4 @@
 //! DRR (Deficit Round Robin) qdisc and class support.
-//!
-//! DRR is our root qdisc, chosen because it allows an unlimited number of classes
-//! to be created dynamically with minimal overhead. Each class can have its own
-//! qdisc chain (TBF → netem) for per-destination impairments.
 
 use rtnetlink::packet_core::{
     NLM_F_ACK, NLM_F_CREATE, NLM_F_EXCL, NLM_F_REPLACE, NLM_F_REQUEST, NetlinkMessage,
@@ -28,7 +24,7 @@ const TCA_DRR_QUANTUM: u16 = 1;
 /// The quantum must be at least as large as the maximum packet size (MTU) to ensure packets
 /// can always be dequeued. 1MB is large enough to handle any reasonable packet while still
 /// being well within safe bounds.
-pub const DRR_DEFAULT_QUANTUM: u32 = u32::MAX; // 1 MB
+pub const DRR_DEFAULT_QUANTUM: u32 = u32::MAX; // 4GiB
 
 /// Builder for creating a DRR (Deficit Round Robin) root qdisc.
 ///
@@ -88,13 +84,13 @@ impl QdiscDrrRequest {
 /// # Example
 ///
 /// ```
-/// use msg_sim::tc::handle::{CLASS_MINOR_OFFSET, QdiscRequestInner};
+/// use msg_sim::tc::handle::{ID_OFFSET, QdiscRequestInner};
 /// use msg_sim::tc::drr::DrrClassRequest;
 /// use rtnetlink::packet_route::tc::TcHandle;
 ///
 /// let if_index = 1; // Network interface index
 /// // Create class for traffic to peer 2
-/// let class_minor = CLASS_MINOR_OFFSET + 2; // 12
+/// let class_minor = ID_OFFSET + 2; // 12
 /// let request = DrrClassRequest::new(
 ///     QdiscRequestInner::new(if_index)
 ///         .with_parent(TcHandle::from(0x0001_0000))  // Parent is DRR root (1:0)

msg-sim/src/tc/drr.rs

@@ -21,6 +21,9 @@ const ETH_P_IP: u16 = nix::libc::ETH_P_IP as u16;
 /// EtherType for IPv6 packets (0x86DD).
 const ETH_P_IPV6: u16 = nix::libc::ETH_P_IPV6 as u16;
 
+/// EtherType for matching all protocols supported by Ethernet.
+const ETH_P_ALL: u16 = nix::libc::ETH_P_ALL as u16;
+
 /// Compute an IPv4 netmask from a prefix length.
 fn ipv4_mask(prefix_len: u8) -> Ipv4Addr {
     if prefix_len == 0 {
@@ -82,9 +85,9 @@ pub struct FlowerFilterRequest {
 }
 
 impl FlowerFilterRequest {
-    /// Create a new flower filter for the given destination IP.
+    /// Create a new flower filter for the given destination IP
     ///
-    /// By default, uses /32 (exact match) for IPv4 or /128 for IPv6.
+    /// By default, uses /32 (exact match) for IPv4 or /128 for IPv6 and root class id.
     pub fn new(inner: QdiscRequestInner, destination: IpAddr) -> Self {
         let default_mask = match destination {
             IpAddr::V4(_) => 32,
@@ -95,7 +98,7 @@ impl FlowerFilterRequest {
             destination,
             mask: default_mask,
             // Default class ID will be set by caller
-            class_id: 0,
+            class_id: u32::MAX, // Root class id
         }
     }
 
@@ -213,6 +216,14 @@ struct TcU32Sel {
     // Followed by nkeys * tc_u32_key structures
 }
 
+impl TcU32Sel {
+    /// Returns an instance of [`Self`] with all fields set to zero.
+    #[allow(dead_code)]
+    fn zero() -> Self {
+        Self { flags: 0, offmask: 0, offshift: 0, nkeys: 0, off: 0, offoff: 0, hoff: 0, hmask: 0 }
+    }
+}
+
 /// The kernel's `tc_u32_key` structure for u32 matching.
 #[derive(Debug, Clone, Copy, Default)]
 struct TcU32Key {
@@ -226,6 +237,13 @@ struct TcU32Key {
     offmask: i32,
 }
 
+impl TcU32Key {
+    /// Returns an instance of [`Self`] with all fields set to zero.
+    fn zero() -> Self {
+        Self { mask: 0, val: 0, off: 0, offmask: 0 }
+    }
+}
+
 /// Builder for creating a u32 catch-all filter.
 ///
 /// The u32 filter with `match u32 0 0` matches all packets. This is used as a catch-all
@@ -241,8 +259,10 @@ struct TcU32Key {
 /// # Why u32 Instead of matchall
 ///
 /// The `matchall` filter requires the `cls_matchall` kernel module which may not be
-/// available on all systems. The `u32` filter is more universally supported and can
-/// achieve the same effect with `match u32 0 0` (which always matches).
+/// available on all systems (e.g., minimal/embedded kernels, older distros, or containers
+/// without the module loaded). The `u32` filter is part of `cls_u32`, which is almost
+/// always built-in, and can achieve the same effect with `match u32 0 0` (mask=0 matches
+/// everything).
 ///
 /// # Priority System
 ///
@@ -280,8 +300,8 @@ impl U32CatchallFilterRequest {
     pub fn new(inner: QdiscRequestInner) -> Self {
         Self {
             inner,
-            class_id: 0,
-            priority: 65535, // Lowest priority = checked last
+            class_id: u32::MAX, // Root class id.
+            priority: 65535,    // Lowest priority = checked last
         }
     }
 
@@ -304,10 +324,7 @@ impl U32CatchallFilterRequest {
         let mut tc_msg = TcMessage::with_index(self.inner.interface_index);
         tc_msg.header.parent = self.inner.parent;
         tc_msg.header.handle = TcHandle::from(0u32);
-        // The info field contains the priority (in upper 16 bits) and protocol (in lower 16 bits)
-        // ETH_P_ALL (0x0003) matches all protocols
-        let eth_p_all: u16 = 0x0003;
-        tc_msg.header.info = ((self.priority as u32) << 16) | (eth_p_all.to_be() as u32);
+        tc_msg.header.info = ((self.priority as u32) << 16) | (ETH_P_ALL.to_be() as u32);
 
         tc_msg.attributes.push(TcAttribute::Kind("u32".to_string()));
 
@@ -320,7 +337,7 @@ impl U32CatchallFilterRequest {
 
         // Mask of 0 means "don't care".
         // Value doesn't matter when mask is 0
-        let key = TcU32Key::default();
+        let key = TcU32Key::zero();
 
         // Serialize selector + key
         let mut sel_bytes = Vec::with_capacity(size_of::<TcU32Sel>() + size_of::<TcU32Key>());

msg-sim/src/tc/filter.rs

@@ -7,9 +7,9 @@ use rtnetlink::packet_route::tc::TcHandle;
 
 /// The offset added to peer IDs to compute DRR class minor numbers.
 ///
-/// For peer ID `N`, the class minor is `CLASS_MINOR_OFFSET + N`.
-/// This keeps class 1:1 reserved as the default (unimpaired) class.
-pub const CLASS_MINOR_OFFSET: u32 = 10;
+/// For peer ID `N`, the class minor/major (depending on the qdisc level) is `ID_OFFSET + N`. This
+/// keeps class 1:1 reserved as the default (unimpaired) class.
+pub const ID_OFFSET: u32 = 10;
 
 /// Offset for netem qdisc major numbers (separate from TBF to avoid collisions).
 pub const NETEM_MAJOR_OFFSET: u32 = 20;
@@ -60,7 +60,7 @@ impl QdiscRequestInner {
 /// assert_eq!(drr_class_handle(2), 0x0001_000C); // 1:12
 /// ```
 pub fn drr_class_handle(dest_peer_id: usize) -> u32 {
-    let minor = CLASS_MINOR_OFFSET + dest_peer_id as u32;
+    let minor = ID_OFFSET + dest_peer_id as u32;
     (1 << 16) | minor
 }
 
@@ -78,7 +78,7 @@ pub fn drr_class_handle(dest_peer_id: usize) -> u32 {
 /// assert_eq!(tbf_handle(2), 0x000C_0000); // 12:0
 /// ```
 pub fn tbf_handle(dest_peer_id: usize) -> u32 {
-    let major = CLASS_MINOR_OFFSET + dest_peer_id as u32;
+    let major = ID_OFFSET + dest_peer_id as u32;
     major << 16 // minor must be 0 for qdiscs
 }
 

msg-sim/src/tc/handle.rs

@@ -57,7 +57,7 @@ pub struct LinkImpairment {
     ///
     /// When this limit is reached, additional packets are dropped. Default is 1000,
     /// which matches netem's default. Increase for high-bandwidth, high-latency links
-    /// to avoid artificial drops.
+    /// to avoid unintended drops.
     pub netem_limit: u32,
 
     /// Packet loss percentage (0.0 to 100.0).
@@ -251,10 +251,15 @@ impl LinkImpairment {
         if let Some(burst_kib) = self.burst_kib {
             burst_kib * 1024
         } else if let Some(bandwidth_mbit) = self.bandwidth_mbit_s {
-            // Compute default: max(bandwidth/8 seconds, 10 MTU packets)
+            // TBF requires a non-zero burst: it defines how many tokens can accumulate
+            // in the bucket. With burst=0, no packets could ever be sent.
+            //
+            // Default: max(1/8 second of bandwidth, 10 MTU packets). This is large
+            // enough to handle typical traffic bursts without being so large that
+            // rate limiting becomes ineffective.
             let bandwidth_bytes_per_sec = (bandwidth_mbit * 1_000_000.0 / 8.0) as u32;
             let one_eighth_second = bandwidth_bytes_per_sec / 8;
-            let ten_packets = MTU_ETHERNET * 10; // 10 MTU-sized packets
+            let ten_packets = MTU_ETHERNET * 10;
             std::cmp::max(one_eighth_second, ten_packets)
         } else {
             // No bandwidth limit, burst is irrelevant
@@ -272,8 +277,7 @@ impl LinkImpairment {
         let queue_latency_ms = self.tbf_queue_latency_ms.unwrap_or(200);
         let burst_bytes = self.effective_burst_bytes();
 
-        if let Some(bandwidth_mbit) = self.bandwidth_mbit_s {
-            let rate_bytes_per_sec = (bandwidth_mbit * 1_000_000.0 / 8.0) as u32;
+        if let Some(rate_bytes_per_sec) = self.bandwidth_bytes_per_sec() {
             let rate_bytes_per_ms = rate_bytes_per_sec / 1000;
             rate_bytes_per_ms * queue_latency_ms + burst_bytes
         } else {
@@ -285,7 +289,7 @@ impl LinkImpairment {
     /// Compute the bandwidth rate in bytes per second.
     ///
     /// Returns `None` if no bandwidth limit is configured.
-    pub fn bandwidth_bytes_per_sec(&self) -> Option<u64> {
-        self.bandwidth_mbit_s.map(|mbit| (mbit * 1_000_000.0 / 8.0) as u64)
+    pub fn bandwidth_bytes_per_sec(&self) -> Option<u32> {
+        self.bandwidth_mbit_s.map(|mbit| (mbit * 1_000_000.0 / 8.0) as u32)
     }
 }

msg-sim/src/tc/impairment.rs

@@ -58,8 +58,8 @@
 //!
 //! | Component        | Handle          | Example (peer_id=2) |
 //! |------------------|-----------------|---------------------|
-//! | DRR root         | `1:0`           | `1:0`               |
-//! | Default class    | `1:1`           | `1:1`               |
+//! | DRR root         | `1:0`           | `N/A`               |
+//! | Default class    | `1:1`           | `N/A`               |
 //! | Per-dest class   | `1:(10+id)`     | `1:12`              |
 //! | TBF qdisc        | `(10+id):0`     | `12:0`              |
 //! | Netem qdisc      | `(20+id):0`     | `22:0`              |

msg-sim/src/tc/mod.rs

@@ -21,6 +21,8 @@ use rtnetlink::packet_core::{DefaultNla, NLA_HEADER_SIZE};
 /// └─────────────────────────────────────────┘
 /// ```
 ///
+/// Reference: <linux/netlink.h>
+///
 /// # Returns
 ///
 /// A byte vector containing the complete NLA (header + value + padding).