From 3d5a8fae144d851d02a559ce1af3e90a247b9674 Mon Sep 17 00:00:00 2001 From: Nils Andreas Svee Date: Tue, 11 Aug 2026 19:24:00 +0700 Subject: [PATCH] fix(client): align DSCP wire semantics --- crates/irtt-client/src/async_client.rs | 58 +++++++++-------- crates/irtt-client/src/async_client/tests.rs | 28 +++++---- crates/irtt-client/src/client.rs | 62 ++++++++++--------- crates/irtt-client/src/client/tests/close.rs | 12 ++-- crates/irtt-client/src/client/tests/config.rs | 11 +++- crates/irtt-client/src/client/tests/dscp.rs | 24 ++++--- .../src/client/tests/negotiation.rs | 31 ++++++++-- .../irtt-client/src/client/tests/no_test.rs | 2 +- crates/irtt-client/src/client/tests/open.rs | 16 ++--- .../tests/probes_replies/reply_metadata.rs | 2 +- crates/irtt-client/src/config.rs | 6 +- crates/irtt-client/src/receive/linux.rs | 6 +- crates/irtt-client/src/session/machine.rs | 3 +- crates/irtt-client/src/session/negotiation.rs | 36 +++++++---- crates/irtt-client/src/socket_options.rs | 54 ++++++++++------ crates/irtt-client/tests/protocol_options.rs | 6 +- crates/irtt-client/tests/support/mod.rs | 17 ++++- crates/irtt-proto/src/params.rs | 15 ++++- 18 files changed, 249 insertions(+), 140 deletions(-) diff --git a/crates/irtt-client/src/async_client.rs b/crates/irtt-client/src/async_client.rs index f693b85..22220e6 100644 --- a/crates/irtt-client/src/async_client.rs +++ b/crates/irtt-client/src/async_client.rs @@ -30,7 +30,7 @@ use crate::{ PreparedProbe, SessionMachine, TimeoutBatch, MAX_OPEN_PACKET_SIZE, }, socket::{connect_tokio_udp_socket, resolve_remote_tokio, validate_open_timeouts}, - socket_options::{apply_dscp_to_tokio_socket, clear_dscp_on_tokio_socket}, + socket_options::{apply_traffic_class_to_tokio_socket, clear_dscp_on_tokio_socket}, timing::ClientTimestamp, }; @@ -49,7 +49,7 @@ struct PreparedAsyncOpen { machine: PreparedOpenAcceptance, schedule: Option, recv_buffer_len: Option, - negotiated_dscp: Option, + negotiated_traffic_class: Option, } #[derive(Debug)] @@ -219,7 +219,7 @@ pub struct AsyncClient { schedule: Option, remote: SocketAddr, recv_buffer: Vec, - applied_dscp: Option, + applied_traffic_class: Option, prepared_open: Option, prepared_probe: Option, #[cfg(test)] @@ -247,7 +247,7 @@ impl AsyncClient { schedule: None, remote, recv_buffer: vec![0_u8; recv_buffer_size(false, None)?], - applied_dscp: None, + applied_traffic_class: None, prepared_open: Some(prepared_open), prepared_probe: None, #[cfg(test)] @@ -618,7 +618,7 @@ impl AsyncClient { self.schedule = None; self.prepared_probe = None; if self.clear_peer_close_dscp().is_ok() { - self.applied_dscp = None; + self.applied_traffic_class = None; } } return Poll::Ready(Ok(events)); @@ -641,7 +641,7 @@ impl AsyncClient { source, })); } - let previous_dscp = self.applied_dscp; + let previous_traffic_class = self.applied_traffic_class; let expected_bytes = prepared.bytes.len(); #[cfg(test)] @@ -661,7 +661,7 @@ impl AsyncClient { let mut rollback = DscpRollback::armed( &self.socket, self.remote, - previous_dscp, + previous_traffic_class, #[cfg(test)] &self.test_hooks.fail_dscp_restore, ); @@ -698,7 +698,7 @@ impl AsyncClient { rollback.disarm(); self.schedule = None; self.prepared_probe = None; - self.applied_dscp = None; + self.applied_traffic_class = None; if let Err(error) = validate_datagram_length(expected_bytes, bytes) { return Poll::Ready(Err(error)); @@ -811,7 +811,7 @@ impl AsyncClient { machine, schedule: None, recv_buffer_len: None, - negotiated_dscp: None, + negotiated_traffic_class: None, }); }; let schedule = match ProbeSchedule::new(opened_at.mono, negotiated) { @@ -826,12 +826,12 @@ impl AsyncClient { return Err(Box::new(PreparedAsyncOpenFailure { primary, machine })); } }; - let negotiated_dscp = match u8::try_from(negotiated.params.dscp) { - Ok(dscp) => dscp, + let negotiated_traffic_class = match u8::try_from(negotiated.params.dscp) { + Ok(traffic_class) => traffic_class, Err(_) => { return Err(Box::new(PreparedAsyncOpenFailure { primary: ClientError::InvalidConfig { - reason: "negotiated dscp must be in range 0..=63".to_owned(), + reason: "negotiated dscp must be in range 0..=255".to_owned(), }, machine, })); @@ -855,12 +855,16 @@ impl AsyncClient { source: io::Error::other("injected negotiated DSCP failure"), }) } else { - apply_dscp_to_tokio_socket(&self.socket, self.remote, negotiated_dscp) + apply_traffic_class_to_tokio_socket(&self.socket, self.remote, negotiated_traffic_class) }; #[cfg(not(test))] - let dscp_result = apply_dscp_to_tokio_socket(&self.socket, self.remote, negotiated_dscp); + let dscp_result = apply_traffic_class_to_tokio_socket( + &self.socket, + self.remote, + negotiated_traffic_class, + ); if let Err(primary) = dscp_result { - self.restore_dscp_best_effort(self.applied_dscp); + self.restore_dscp_best_effort(self.applied_traffic_class); return Err(Box::new(PreparedAsyncOpenFailure { primary, machine })); } @@ -868,7 +872,7 @@ impl AsyncClient { machine, schedule: Some(schedule), recv_buffer_len: Some(recv_buffer_len), - negotiated_dscp: Some(negotiated_dscp), + negotiated_traffic_class: Some(negotiated_traffic_class), }) } @@ -878,7 +882,7 @@ impl AsyncClient { } let outcome = self.machine.commit_open(prepared.machine); self.schedule = prepared.schedule; - self.applied_dscp = prepared.negotiated_dscp; + self.applied_traffic_class = prepared.negotiated_traffic_class; self.prepared_open = None; outcome } @@ -950,9 +954,11 @@ impl AsyncClient { clear_dscp_on_tokio_socket(&self.socket, self.remote) } - fn restore_dscp_best_effort(&self, previous_dscp: Option) { - let _ = match previous_dscp { - Some(dscp) => apply_dscp_to_tokio_socket(&self.socket, self.remote, dscp), + fn restore_dscp_best_effort(&self, previous_traffic_class: Option) { + let _ = match previous_traffic_class { + Some(traffic_class) => { + apply_traffic_class_to_tokio_socket(&self.socket, self.remote, traffic_class) + } None => clear_dscp_on_tokio_socket(&self.socket, self.remote), }; } @@ -961,7 +967,7 @@ impl AsyncClient { struct DscpRollback<'a> { socket: &'a tokio::net::UdpSocket, remote: SocketAddr, - previous_dscp: Option, + previous_traffic_class: Option, armed: bool, #[cfg(test)] fail_restore: &'a Cell, @@ -971,13 +977,13 @@ impl<'a> DscpRollback<'a> { fn armed( socket: &'a tokio::net::UdpSocket, remote: SocketAddr, - previous_dscp: Option, + previous_traffic_class: Option, #[cfg(test)] fail_restore: &'a Cell, ) -> Self { Self { socket, remote, - previous_dscp, + previous_traffic_class, armed: true, #[cfg(test)] fail_restore, @@ -996,8 +1002,10 @@ impl<'a> DscpRollback<'a> { source: io::Error::other("injected DSCP restore failure"), }); } - match self.previous_dscp { - Some(dscp) => apply_dscp_to_tokio_socket(self.socket, self.remote, dscp)?, + match self.previous_traffic_class { + Some(traffic_class) => { + apply_traffic_class_to_tokio_socket(self.socket, self.remote, traffic_class)? + } None => clear_dscp_on_tokio_socket(self.socket, self.remote)?, } self.armed = false; diff --git a/crates/irtt-client/src/async_client/tests.rs b/crates/irtt-client/src/async_client/tests.rs index 779d84f..5448b51 100644 --- a/crates/irtt-client/src/async_client/tests.rs +++ b/crates/irtt-client/src/async_client/tests.rs @@ -493,7 +493,7 @@ fn opening_cancellation_before_submission_is_transactional() { assert!(client.machine.prepare_open_request().is_ok()); assert!(client.prepared_open.is_some()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); }); assert!(server.finish().is_empty()); } @@ -527,7 +527,7 @@ fn opening_cancellation_after_submission_can_retry() { assert!(client.machine.prepare_open_request().is_ok()); assert!(client.prepared_open.is_some()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); assert!(matches!( client.open().await.unwrap(), OpenOutcome::Started { .. } @@ -555,7 +555,7 @@ fn adapter_failure_sends_preencoded_cleanup_without_committing_open() { )); assert!(client.machine.prepare_open_request().is_ok()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); }); assert_eq!(server.finish().len(), 2); } @@ -598,7 +598,7 @@ fn no_test_open_has_no_schedule_or_dscp_state() { )); assert!(client.prepared_open.is_none()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); assert!(client.is_run_complete()); assert!(matches!( client.close().await, @@ -879,7 +879,7 @@ fn peer_close_dscp_failure_preserves_events_and_logical_ownership() { assert!(client.is_run_complete()); assert!(client.schedule.is_none()); assert!(client.prepared_probe.is_none()); - assert_eq!(client.applied_dscp, Some(46)); + assert_eq!(client.applied_traffic_class, Some(184)); assert_eq!(client.next_send_deadline(), None); assert_eq!(socket_dscp(&client), 46); }); @@ -936,7 +936,7 @@ fn close_cancellation_and_would_block_restore_dscp_before_suspend() { assert!(client.machine.is_open()); assert!(client.schedule.is_some()); assert_eq!(client.next_send_deadline(), deadline); - assert_eq!(client.applied_dscp, Some(46)); + assert_eq!(client.applied_traffic_class, Some(184)); assert_eq!(client.test_hooks.send_attempts.get(), attempts); assert_eq!(socket_dscp(&client), 46); @@ -947,7 +947,7 @@ fn close_cancellation_and_would_block_restore_dscp_before_suspend() { drop(close); assert!(client.machine.is_open()); assert!(client.schedule.is_some()); - assert_eq!(client.applied_dscp, Some(46)); + assert_eq!(client.applied_traffic_class, Some(184)); assert_eq!(socket_dscp(&client), 46); assert!(matches!( @@ -955,7 +955,7 @@ fn close_cancellation_and_would_block_restore_dscp_before_suspend() { [ClientEvent::SessionClosed { .. }] )); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); }); assert_eq!(server.finish().len(), 2); } @@ -972,7 +972,7 @@ fn close_errors_preserve_open_state_and_primary_send_error() { assert!(client.machine.is_open()); assert!(client.schedule.is_some()); assert_eq!(client.next_send_deadline(), deadline); - assert_eq!(client.applied_dscp, Some(46)); + assert_eq!(client.applied_traffic_class, Some(184)); assert_eq!(socket_dscp(&client), 46); client.test_hooks.fail_dscp_restore.set(true); @@ -1001,7 +1001,7 @@ fn close_would_block_restore_failure_stops_without_committing() { )); assert!(client.machine.is_open()); assert!(client.schedule.is_some()); - assert_eq!(client.applied_dscp, Some(46)); + assert_eq!(client.applied_traffic_class, Some(184)); }); assert_eq!(server.finish().len(), 1); } @@ -1027,7 +1027,7 @@ fn successful_short_close_commits_before_length_error() { assert!(!client.machine.is_open()); assert!(client.schedule.is_none()); assert!(client.prepared_probe.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); assert!(matches!( client.machine.prepare_close(), Err(ClientError::AlreadyClosed) @@ -1085,7 +1085,11 @@ fn blocking_and_async_hmac_dscp_lifecycle_are_semantically_equivalent() { open_negotiated(&blocking_open), open_negotiated(&async_open) ); - assert_eq!(open_negotiated(&async_open).params.dscp, 46); + assert_eq!( + open_negotiated(&async_open).params.dscp, + 184, + "negotiated Params::dscp is the raw wire byte for codepoint 46" + ); assert_matching_event_shape(&blocking_sent[0], &async_sent[0]); assert_matching_event_shape(&blocking_reply[0], &async_reply[0]); assert_matching_event_shape(&blocking_close[0], &async_close[0]); diff --git a/crates/irtt-client/src/client.rs b/crates/irtt-client/src/client.rs index 7ed52c9..064f271 100644 --- a/crates/irtt-client/src/client.rs +++ b/crates/irtt-client/src/client.rs @@ -20,7 +20,7 @@ use crate::{ SessionMachine, MAX_OPEN_PACKET_SIZE, }, socket::{connect_udp_socket, resolve_remote, validate_open_timeouts}, - socket_options::{apply_dscp_to_socket, clear_dscp_on_socket}, + socket_options::{apply_traffic_class_to_socket, clear_dscp_on_socket}, timing::ClientTimestamp, }; @@ -42,8 +42,8 @@ struct PreparedClientOpen { machine: PreparedOpenAcceptance, schedule: Option, recv_buffer_len: Option, - negotiated_dscp: Option, - previous_dscp: Option, + negotiated_traffic_class: Option, + previous_traffic_class: Option, post_open_recv_timeout: Option, } @@ -103,7 +103,7 @@ pub struct Client { socket: UdpSocket, remote: SocketAddr, recv_buffer: Vec, - applied_dscp: Option, + applied_traffic_class: Option, #[cfg(test)] test_hooks: ClientTestHooks, #[cfg(test)] @@ -132,7 +132,7 @@ impl Client { socket, remote, recv_buffer: vec![0_u8; recv_buffer_size(false, None)?], - applied_dscp: None, + applied_traffic_class: None, #[cfg(test)] test_hooks: ClientTestHooks::default(), #[cfg(test)] @@ -192,14 +192,14 @@ impl Client { operation: "close event result", source, })?; - let previous_dscp = self.applied_dscp; + let previous_traffic_class = self.applied_traffic_class; let expected_bytes = prepared.bytes.len(); self.clear_close_dscp()?; let bytes = match self.send_close_datagram(prepared.bytes) { Ok(bytes) => bytes, Err(err) => { - self.restore_dscp_best_effort(previous_dscp); + self.restore_dscp_best_effort(previous_traffic_class); return Err(ClientError::Socket(err)); } }; @@ -213,7 +213,7 @@ impl Client { .unwrap_or_else(ClientTimestamp::now); let event = self.runtime.commit_local_close(prepared.commit, sent_at); self.schedule = None; - self.applied_dscp = None; + self.applied_traffic_class = None; validate_datagram_length(expected_bytes, bytes)?; events.push(event); @@ -275,7 +275,7 @@ impl Client { if self.runtime.is_peer_closed() { self.schedule = None; if self.clear_close_dscp().is_ok() { - self.applied_dscp = None; + self.applied_traffic_class = None; } } Ok(events) @@ -521,8 +521,8 @@ impl Client { machine, schedule: None, recv_buffer_len: None, - negotiated_dscp: None, - previous_dscp: self.applied_dscp, + negotiated_traffic_class: None, + previous_traffic_class: self.applied_traffic_class, post_open_recv_timeout: self.runtime.config().socket_config.recv_timeout, }); }; @@ -540,12 +540,12 @@ impl Client { .recv_buffer_len_override .get() .unwrap_or(recv_buffer_len); - let negotiated_dscp = match u8::try_from(negotiated.params.dscp) { - Ok(dscp) => dscp, + let negotiated_traffic_class = match u8::try_from(negotiated.params.dscp) { + Ok(traffic_class) => traffic_class, Err(_) => { return Err(Box::new(PreparedClientOpenFailure { primary: ClientError::InvalidConfig { - reason: "negotiated dscp must be in range 0..=63".to_owned(), + reason: "negotiated dscp must be in range 0..=255".to_owned(), }, machine, })); @@ -556,8 +556,8 @@ impl Client { machine, schedule: Some(schedule), recv_buffer_len: Some(recv_buffer_len), - negotiated_dscp: Some(negotiated_dscp), - previous_dscp: self.applied_dscp, + negotiated_traffic_class: Some(negotiated_traffic_class), + previous_traffic_class: self.applied_traffic_class, post_open_recv_timeout: self.runtime.config().socket_config.recv_timeout, }) } @@ -570,12 +570,14 @@ impl Client { machine, schedule, recv_buffer_len, - negotiated_dscp, - previous_dscp, + negotiated_traffic_class, + previous_traffic_class, post_open_recv_timeout, } = prepared; - if let (Some(recv_buffer_len), Some(negotiated_dscp)) = (recv_buffer_len, negotiated_dscp) { + if let (Some(recv_buffer_len), Some(negotiated_traffic_class)) = + (recv_buffer_len, negotiated_traffic_class) + { let previous_len = self.recv_buffer.len(); let additional = recv_buffer_len.saturating_sub(previous_len); if let Err(source) = self.recv_buffer.try_reserve(additional) { @@ -591,23 +593,23 @@ impl Client { self.test_hooks .prepared_active_session_before_dscp .set(machine.has_prepared_active_session()); - if let Err(primary) = self.apply_open_dscp(negotiated_dscp) { + if let Err(primary) = self.apply_open_dscp(negotiated_traffic_class) { self.recv_buffer.truncate(previous_len); - self.restore_dscp_best_effort(previous_dscp); + self.restore_dscp_best_effort(previous_traffic_class); self.send_cleanup_close_best_effort(machine.cleanup_close_packet()); return Err(primary); } if let Err(source) = self.restore_open_read_timeout(post_open_recv_timeout) { let primary = ClientError::ReadTimeoutRestore { source }; self.recv_buffer.truncate(previous_len); - self.restore_dscp_best_effort(previous_dscp); + self.restore_dscp_best_effort(previous_traffic_class); self.send_cleanup_close_best_effort(machine.cleanup_close_packet()); return Err(primary); } let outcome = self.runtime.commit_open(machine); self.schedule = schedule; - self.applied_dscp = Some(negotiated_dscp); + self.applied_traffic_class = Some(negotiated_traffic_class); Ok(outcome) } else { debug_assert!(schedule.is_none()); @@ -616,18 +618,20 @@ impl Client { } let outcome = self.runtime.commit_open(machine); self.schedule = None; - self.applied_dscp = None; + self.applied_traffic_class = None; Ok(outcome) } } - fn restore_dscp_best_effort(&self, previous_dscp: Option) { + fn restore_dscp_best_effort(&self, previous_traffic_class: Option) { #[cfg(test)] if self.test_hooks.fail_dscp_restore.replace(false) { return; } - let _ = match previous_dscp { - Some(dscp) => apply_dscp_to_socket(&self.socket, self.remote, dscp), + let _ = match previous_traffic_class { + Some(traffic_class) => { + apply_traffic_class_to_socket(&self.socket, self.remote, traffic_class) + } None => clear_dscp_on_socket(&self.socket, self.remote), }; } @@ -654,7 +658,7 @@ impl Client { } } - fn apply_open_dscp(&self, dscp: u8) -> Result<(), ClientError> { + fn apply_open_dscp(&self, traffic_class: u8) -> Result<(), ClientError> { #[cfg(test)] if self.test_hooks.fail_open_dscp.replace(false) { return Err(ClientError::SocketOption { @@ -663,7 +667,7 @@ impl Client { source: io::Error::other("injected negotiated DSCP failure"), }); } - apply_dscp_to_socket(&self.socket, self.remote, dscp) + apply_traffic_class_to_socket(&self.socket, self.remote, traffic_class) } fn restore_open_read_timeout(&self, timeout: Option) -> io::Result<()> { diff --git a/crates/irtt-client/src/client/tests/close.rs b/crates/irtt-client/src/client/tests/close.rs index 79813cb..46660af 100644 --- a/crates/irtt-client/src/client/tests/close.rs +++ b/crates/irtt-client/src/client/tests/close.rs @@ -45,7 +45,7 @@ fn close_sends_one_close_packet_with_negotiated_token() { )); assert!(!client.runtime.is_open()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); let packets: Vec<_> = server.rx.iter().take(2).collect(); let close = &packets[1]; assert_eq!(close[3], flags::FLAG_CLOSE); @@ -82,7 +82,7 @@ fn close_event_reservation_failure_precedes_dscp_clear_and_send() { assert!(client.test_hooks.fail_close_dscp_clear.get()); assert!(client.runtime.is_open()); assert!(client.schedule.is_some()); - assert_eq!(client.applied_dscp, Some(0)); + assert_eq!(client.applied_traffic_class, Some(0)); client.test_hooks.fail_close_dscp_clear.set(false); client.close().unwrap(); @@ -114,7 +114,7 @@ fn close_dscp_clear_failure_precedes_send_and_preserves_open_state() { assert_eq!(client.test_hooks.close_send_attempts.get(), 0); assert!(client.runtime.is_open()); assert!(client.schedule.is_some()); - assert_eq!(client.applied_dscp, Some(0)); + assert_eq!(client.applied_traffic_class, Some(0)); client.close().unwrap(); assert_eq!(client.test_hooks.close_send_attempts.get(), 1); @@ -147,7 +147,7 @@ fn close_send_failure_leaves_machine_and_schedule_open_for_retry() { assert_eq!(client.test_hooks.close_sent_at.get(), Some(sent_at)); assert!(client.runtime.is_open()); assert!(client.schedule.is_some()); - assert_eq!(client.applied_dscp, Some(0)); + assert_eq!(client.applied_traffic_class, Some(0)); assert_eq!(client.test_hooks.close_send_attempts.get(), 1); assert!(matches!( @@ -196,7 +196,7 @@ fn short_successful_close_commits_before_length_mismatch() { )); assert!(!client.runtime.is_open()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); assert_eq!(client.test_hooks.close_sent_at.get(), None); assert!(matches!(client.close(), Err(ClientError::AlreadyClosed))); server.join(); @@ -351,7 +351,7 @@ fn peer_close_dscp_cleanup_failure_preserves_events_and_closed_state() { )); assert!(client.is_peer_closed()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, Some(0)); + assert_eq!(client.applied_traffic_class, Some(0)); assert_eq!(client.test_hooks.close_send_attempts.get(), 0); let open = server.rx.recv_timeout(Duration::from_millis(100)).unwrap(); let echo = server.rx.recv_timeout(Duration::from_millis(100)).unwrap(); diff --git a/crates/irtt-client/src/client/tests/config.rs b/crates/irtt-client/src/client/tests/config.rs index b7f3f5b..dcdcbee 100644 --- a/crates/irtt-client/src/client/tests/config.rs +++ b/crates/irtt-client/src/client/tests/config.rs @@ -42,7 +42,10 @@ fn params_from_config_maps_compatibility_fields() { assert_eq!(params.received_stats, ReceivedStats::Window); assert_eq!(params.stamp_at, StampAt::Midpoint); assert_eq!(params.clock, Clock::Wall); - assert_eq!(params.dscp, 46, "config DSCP codepoint must not be shifted"); + assert_eq!( + params.dscp, 184, + "config DSCP codepoint 46 must be shifted into raw wire byte 184" + ); assert_eq!( params.server_fill.as_ref().map(|fill| fill.value.as_str()), Some("rand") @@ -67,7 +70,11 @@ fn params_from_config_accepts_boundary_values() { dscp: 63, ..ClientConfig::default() }; - assert_eq!(params_from_config(&config).unwrap().dscp, 63); + assert_eq!( + params_from_config(&config).unwrap().dscp, + 252, + "boundary codepoint 63 must be shifted into raw wire byte 252" + ); } #[test] diff --git a/crates/irtt-client/src/client/tests/dscp.rs b/crates/irtt-client/src/client/tests/dscp.rs index c98e876..dd1301a 100644 --- a/crates/irtt-client/src/client/tests/dscp.rs +++ b/crates/irtt-client/src/client/tests/dscp.rs @@ -9,9 +9,11 @@ use crate::socket_options::socket_traffic_class; target_os = "illumos", target_os = "haiku", )))] -fn normal_open_applies_negotiated_dscp_after_open_and_close_clears_it() { +fn normal_open_applies_negotiated_traffic_class_after_open_and_close_clears_it() { let mut params = default_params(); - params.dscp = 46; + // Raw wire TOS/Traffic Class byte for DSCP codepoint 46 (EF): a + // compliant server echoes back the raw byte it received, unchanged. + params.dscp = 184; let server = start_fake_server(move |socket, tx| { let (_, peer) = recv_request(&socket, &tx); let reply = open_reply(FLAG_OPEN | FLAG_REPLY, TOKEN, ¶ms, None); @@ -48,7 +50,7 @@ fn normal_open_applies_negotiated_dscp_after_open_and_close_clears_it() { target_os = "illumos", target_os = "haiku", )))] -fn normal_open_uses_negotiated_dscp_not_requested_dscp() { +fn normal_open_uses_negotiated_traffic_class_not_requested_dscp() { let mut returned = default_params(); returned.dscp = 0; let server = open_success_server(returned); @@ -82,9 +84,10 @@ fn normal_open_uses_negotiated_dscp_not_requested_dscp() { target_os = "illumos", target_os = "haiku", )))] -fn failed_close_send_restores_negotiated_dscp_and_keeps_session_open() { +fn failed_close_send_restores_negotiated_traffic_class_and_keeps_session_open() { let mut params = default_params(); - params.dscp = 46; + // Raw wire TOS/Traffic Class byte for DSCP codepoint 46 (EF). + params.dscp = 184; let server = start_fake_server(move |socket, tx| { let (_, peer) = recv_request(&socket, &tx); socket @@ -106,7 +109,7 @@ fn failed_close_send_restores_negotiated_dscp_and_keeps_session_open() { assert!(matches!(error, ClientError::Socket(_))); assert!(client.runtime.is_open()); assert!(client.schedule.is_some()); - assert_eq!(client.applied_dscp, Some(46)); + assert_eq!(client.applied_traffic_class, Some(184)); assert_eq!( socket_traffic_class(&client.socket, client.remote).unwrap() & 0xfc, 184 @@ -139,7 +142,7 @@ fn failed_close_keeps_send_error_primary_when_dscp_restoration_also_fails() { assert!(error.to_string().contains("injected close send failure")); assert!(client.runtime.is_open()); assert!(client.schedule.is_some()); - assert_eq!(client.applied_dscp, Some(0)); + assert_eq!(client.applied_traffic_class, Some(0)); client.close().unwrap(); server.join(); } @@ -152,9 +155,10 @@ fn failed_close_keeps_send_error_primary_when_dscp_restoration_also_fails() { target_os = "illumos", target_os = "haiku", )))] -fn authenticated_peer_close_clears_schedule_and_negotiated_dscp() { +fn authenticated_peer_close_clears_schedule_and_negotiated_traffic_class() { let mut params = default_params(); - params.dscp = 46; + // Raw wire TOS/Traffic Class byte for DSCP codepoint 46 (EF). + params.dscp = 184; let server = start_fake_server(move |socket, tx| { let (_, peer) = recv_request(&socket, &tx); socket @@ -195,7 +199,7 @@ fn authenticated_peer_close_clears_schedule_and_negotiated_dscp() { ] )); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); assert_eq!( socket_traffic_class(&client.socket, client.remote).unwrap(), 0 diff --git a/crates/irtt-client/src/client/tests/negotiation.rs b/crates/irtt-client/src/client/tests/negotiation.rs index 6ae170a..49bd347 100644 --- a/crates/irtt-client/src/client/tests/negotiation.rs +++ b/crates/irtt-client/src/client/tests/negotiation.rs @@ -33,13 +33,17 @@ fn strict_negotiation_rejects_changed_params() { ..ClientConfig::default() }; let requested = params_from_config(&config).unwrap(); + assert_eq!( + requested.dscp, 184, + "codepoint 46 must become raw wire byte 184" + ); let mut returned = requested.clone(); returned.dscp = 0; assert_eq!( rejection_reason(&requested, &returned, NegotiationPolicy::Strict), NegotiationRestriction::DscpChanged { - requested: requested.dscp, - negotiated: returned.dscp, + requested: 46, + negotiated: 0, } .message() ); @@ -183,17 +187,34 @@ fn strict_negotiation_rejects_negative_returned_length() { #[test] fn loose_negotiation_rejects_runtime_invalid_returned_dscp() { - let config = ClientConfig::default(); + let config = ClientConfig { + dscp: 46, + ..ClientConfig::default() + }; let requested = params_from_config(&config).unwrap(); + assert_eq!( + requested.dscp, 184, + "codepoint 46 must become raw wire byte 184" + ); - for dscp in [-1, 64] { + for dscp in [-1, 256] { let mut returned = requested.clone(); returned.dscp = dscp; assert_eq!( rejection_reason(&requested, &returned, NegotiationPolicy::Loose), - "dscp must be in range 0..=63" + "dscp must be in range 0..=255" ); } + + // A raw returned value of 184 must be accepted even though it exceeds the + // 0..=63 codepoint range; it is the correctly negotiated wire value for + // codepoint 46 and is not subject to the codepoint bound at all. + let returned = requested.clone(); + assert!( + assert_negotiates(&requested, &returned, NegotiationPolicy::Loose) + .restrictions + .is_empty() + ); } #[test] diff --git a/crates/irtt-client/src/client/tests/no_test.rs b/crates/irtt-client/src/client/tests/no_test.rs index 6b64075..450d5d9 100644 --- a/crates/irtt-client/src/client/tests/no_test.rs +++ b/crates/irtt-client/src/client/tests/no_test.rs @@ -13,7 +13,7 @@ fn no_test_success_validates_params() { assert_eq!(negotiated.params, params); assert!(client.next_send_deadline().is_none()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); server.join(); } diff --git a/crates/irtt-client/src/client/tests/open.rs b/crates/irtt-client/src/client/tests/open.rs index e4e7fb6..0c5515c 100644 --- a/crates/irtt-client/src/client/tests/open.rs +++ b/crates/irtt-client/src/client/tests/open.rs @@ -10,7 +10,7 @@ fn successful_open_handshake() { let negotiated = assert_open_started(client.open().unwrap()); assert_eq!(negotiated.params, params); assert!(client.schedule.is_some()); - assert_eq!(client.applied_dscp, Some(0)); + assert_eq!(client.applied_traffic_class, Some(0)); server.join(); } @@ -284,7 +284,7 @@ fn schedule_duration_overflow_cannot_partially_open() { assert!(failure.machine.cleanup_close_packet().is_some()); assert!(client.runtime.prepare_open_request().is_ok()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); assert_eq!(client.recv_buffer.len(), previous_recv_len); server.join(); } @@ -346,7 +346,9 @@ fn cleanup_send_failure_does_not_replace_primary_open_error() { #[test] fn receive_buffer_reservation_failure_precedes_dscp_application() { let mut returned = default_params(); - returned.dscp = 46; + // Raw wire TOS/Traffic Class byte for DSCP codepoint 46 (EF), matching + // what the client below requests so negotiation succeeds unchanged. + returned.dscp = 184; let server = start_fake_server(move |socket, tx| { let (_, peer) = recv_request(&socket, &tx); socket @@ -376,7 +378,7 @@ fn receive_buffer_reservation_failure_precedes_dscp_application() { assert!(client.test_hooks.fail_open_dscp.get()); assert!(client.runtime.prepare_open_request().is_ok()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); server.join(); } @@ -406,7 +408,7 @@ fn dscp_application_failure_leaves_machine_connected() { assert!(client.test_hooks.prepared_active_session_before_dscp.get()); assert!(client.runtime.prepare_open_request().is_ok()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); server.join(); } @@ -432,7 +434,7 @@ fn read_timeout_restoration_failure_leaves_machine_connected() { )); assert!(client.runtime.prepare_open_request().is_ok()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); server.join(); } @@ -458,6 +460,6 @@ fn dscp_rollback_failure_keeps_timeout_restoration_primary() { assert!(matches!(error, ClientError::ReadTimeoutRestore { .. })); assert!(client.runtime.prepare_open_request().is_ok()); assert!(client.schedule.is_none()); - assert_eq!(client.applied_dscp, None); + assert_eq!(client.applied_traffic_class, None); server.join(); } diff --git a/crates/irtt-client/src/client/tests/probes_replies/reply_metadata.rs b/crates/irtt-client/src/client/tests/probes_replies/reply_metadata.rs index a765bed..a6f1d94 100644 --- a/crates/irtt-client/src/client/tests/probes_replies/reply_metadata.rs +++ b/crates/irtt-client/src/client/tests/probes_replies/reply_metadata.rs @@ -14,7 +14,7 @@ fn echo_reply_metadata_propagates_observed_dscp_with_ancillary() { let (size, _) = socket.recv_from(&mut buf).unwrap(); tx.send(buf[..size].to_vec()).unwrap(); let seq = u32::from_le_bytes(buf[12..16].try_into().unwrap()); - crate::socket_options::apply_dscp_to_socket(&socket, peer, 46).unwrap(); + crate::socket_options::apply_traffic_class_to_socket(&socket, peer, 184).unwrap(); let reply_packet = echo_reply_packet(TOKEN, seq, ¶ms, &TimestampFields::default(), None); socket.send_to(&reply_packet, peer).unwrap(); diff --git a/crates/irtt-client/src/config.rs b/crates/irtt-client/src/config.rs index 44a3215..cd69eb3 100644 --- a/crates/irtt-client/src/config.rs +++ b/crates/irtt-client/src/config.rs @@ -12,8 +12,10 @@ use irtt_proto::{Clock, ReceivedStats, StampAt}; pub(crate) const DEFAULT_PORT: u16 = 2112; /// Largest valid DSCP codepoint accepted by client configuration. /// -/// DSCP is a six-bit value in the range `0..=63`. The client maps it into the -/// socket traffic-class field when configuring packet priority. +/// DSCP is a six-bit value in the range `0..=63`. The client shifts it left +/// by two bits into the raw IP TOS / Traffic Class byte carried by +/// [`irtt_proto::Params::dscp`] and applied to the socket during the active +/// test phase. pub const MAX_DSCP_CODEPOINT: u8 = 63; /// Largest IPv4 TTL or IPv6 hop-limit value accepted by client configuration. /// diff --git a/crates/irtt-client/src/receive/linux.rs b/crates/irtt-client/src/receive/linux.rs index 98294b0..db0e6dd 100644 --- a/crates/irtt-client/src/receive/linux.rs +++ b/crates/irtt-client/src/receive/linux.rs @@ -124,7 +124,7 @@ mod tests { use crate::{ event::PacketMeta, receive::{configure_receive_metadata, recv_datagram}, - socket_options::apply_dscp_to_socket, + socket_options::apply_traffic_class_to_socket, timing::ClientTimestamp, }; @@ -277,7 +277,7 @@ mod tests { fn ipv4_traffic_class_metadata_is_observed_when_kernel_provides_it() { let (sender, receiver) = connected_ipv4_loopback_pair(); configure_receive_metadata(&receiver, sender.local_addr().unwrap()).unwrap(); - apply_dscp_to_socket(&sender, receiver.local_addr().unwrap(), 46).unwrap(); + apply_traffic_class_to_socket(&sender, receiver.local_addr().unwrap(), 184).unwrap(); sender.send(b"dscp").unwrap(); let mut buf = [0_u8; 16]; @@ -299,7 +299,7 @@ mod tests { return; }; configure_receive_metadata(&receiver, sender.local_addr().unwrap()).unwrap(); - apply_dscp_to_socket(&sender, receiver.local_addr().unwrap(), 46).unwrap(); + apply_traffic_class_to_socket(&sender, receiver.local_addr().unwrap(), 184).unwrap(); sender.send(b"dscp").unwrap(); let mut buf = [0_u8; 16]; diff --git a/crates/irtt-client/src/session/machine.rs b/crates/irtt-client/src/session/machine.rs index 21550fb..fbf4db2 100644 --- a/crates/irtt-client/src/session/machine.rs +++ b/crates/irtt-client/src/session/machine.rs @@ -18,6 +18,7 @@ use crate::{ metadata::ReceiveMeta, probe::{CompletedSet, PendingMap, PendingProbe, TimedOutMap}, session::{negotiate_params, NegotiatedParams}, + socket_options::dscp_codepoint_to_traffic_class, timing::ClientTimestamp, }; @@ -848,7 +849,7 @@ pub(crate) fn params_from_config(config: &ClientConfig) -> Result> 2, + negotiated: returned.dscp >> 2, }, )?; } @@ -309,10 +318,14 @@ fn validate_duration_restriction(requested: i64, returned: i64) -> Result<(), Cl Ok(()) } +/// Validates a server-returned `Params::dscp`, which is a raw IP TOS / +/// Traffic Class byte and therefore valid across the full `0..=255` wire +/// range, not the `0..=`[`MAX_DSCP_CODEPOINT`](crate::config::MAX_DSCP_CODEPOINT) +/// range of the public codepoint. fn validate_dscp_restriction(returned: i64) -> Result<(), ClientError> { - if !(0..=i64::from(MAX_DSCP_CODEPOINT)).contains(&returned) { + if !(0..=255).contains(&returned) { return Err(ClientError::NegotiationRejected { - reason: format!("dscp must be in range 0..={MAX_DSCP_CODEPOINT}"), + reason: "dscp must be in range 0..=255".to_owned(), }); } @@ -348,7 +361,8 @@ mod tests { received_stats: ReceivedStats::Both, stamp_at: StampAt::Both, clock: Clock::Both, - dscp: 46, + // Raw wire TOS/Traffic Class byte for DSCP codepoint 46 (EF). + dscp: 184, server_fill: Some(ServerFill { value: "rand".to_owned(), }), @@ -402,8 +416,8 @@ mod tests { assert_eq!( rejection_reason(&requested, &returned, NegotiationPolicy::Strict), NegotiationRestriction::DscpChanged { - requested: requested.dscp, - negotiated: returned.dscp, + requested: requested.dscp >> 2, + negotiated: returned.dscp >> 2, } .message() ); diff --git a/crates/irtt-client/src/socket_options.rs b/crates/irtt-client/src/socket_options.rs index bc8a1d0..489cfc9 100644 --- a/crates/irtt-client/src/socket_options.rs +++ b/crates/irtt-client/src/socket_options.rs @@ -10,6 +10,13 @@ use crate::{ error::ClientError, }; +/// Converts a public DSCP codepoint (`0..=`[`MAX_DSCP_CODEPOINT`]) into the +/// raw IP TOS / Traffic Class byte it occupies the upper six bits of. +/// +/// This is the single place `codepoint << 2` is spelled out; both the +/// config-to-wire conversion and any direct codepoint-based socket +/// application go through this function so the shift cannot be duplicated or +/// accidentally applied twice. pub(crate) fn dscp_codepoint_to_traffic_class(dscp: u8) -> Result { if dscp > MAX_DSCP_CODEPOINT { return Err(ClientError::InvalidConfig { @@ -19,13 +26,19 @@ pub(crate) fn dscp_codepoint_to_traffic_class(dscp: u8) -> Result Result<(), ClientError> { - let traffic_class = dscp_codepoint_to_traffic_class(dscp_codepoint)?; - set_socket_traffic_class(socket, remote, traffic_class, "set negotiated DSCP") + set_socket_traffic_class( + socket, + remote, + u32::from(traffic_class), + "set negotiated DSCP", + ) } pub(crate) fn clear_dscp_on_socket( @@ -35,14 +48,20 @@ pub(crate) fn clear_dscp_on_socket( set_socket_traffic_class(socket, remote, 0, "clear DSCP before close") } +/// Applies an already-raw IP TOS / Traffic Class byte to the Tokio socket +/// directly, without any codepoint shift. #[cfg(feature = "tokio")] -pub(crate) fn apply_dscp_to_tokio_socket( +pub(crate) fn apply_traffic_class_to_tokio_socket( socket: &tokio::net::UdpSocket, remote: SocketAddr, - dscp_codepoint: u8, + traffic_class: u8, ) -> Result<(), ClientError> { - let traffic_class = dscp_codepoint_to_traffic_class(dscp_codepoint)?; - set_tokio_socket_traffic_class(socket, remote, traffic_class, "set negotiated DSCP") + set_tokio_socket_traffic_class( + socket, + remote, + u32::from(traffic_class), + "set negotiated DSCP", + ) } #[cfg(feature = "tokio")] @@ -454,10 +473,10 @@ mod tests { let socket = UdpSocket::bind(SocketAddr::from(([127, 0, 0, 1], 0))).unwrap(); socket.connect(remote).unwrap(); - apply_dscp_to_socket(&socket, remote, 46).unwrap(); - let traffic_class = socket_traffic_class(&socket, remote).unwrap(); - assert_eq!(traffic_class & 0xfc, 184); - assert_eq!(traffic_class & 0b11, 0); + // 184 is the raw wire TOS/Traffic Class byte for DSCP codepoint 46 + // (EF); applying it directly must not shift it again. + apply_traffic_class_to_socket(&socket, remote, 184).unwrap(); + assert_eq!(socket_traffic_class(&socket, remote).unwrap(), 184); clear_dscp_on_socket(&socket, remote).unwrap(); assert_eq!(socket_traffic_class(&socket, remote).unwrap(), 0); @@ -487,10 +506,8 @@ mod tests { socket.set_nonblocking(true).unwrap(); let socket = tokio::net::UdpSocket::from_std(socket).unwrap(); - apply_dscp_to_tokio_socket(&socket, remote, 46).unwrap(); - let traffic_class = tokio_socket_traffic_class(&socket, remote).unwrap(); - assert_eq!(traffic_class & 0xfc, 184); - assert_eq!(traffic_class & 0b11, 0); + apply_traffic_class_to_tokio_socket(&socket, remote, 184).unwrap(); + assert_eq!(tokio_socket_traffic_class(&socket, remote).unwrap(), 184); clear_dscp_on_tokio_socket(&socket, remote).unwrap(); assert_eq!(tokio_socket_traffic_class(&socket, remote).unwrap(), 0); @@ -515,7 +532,7 @@ mod tests { return; }; - apply_dscp_to_socket(&socket, remote, 46).unwrap(); + apply_traffic_class_to_socket(&socket, remote, 184).unwrap(); let traffic_class = match socket_traffic_class(&socket, remote) { Ok(traffic_class) => traffic_class, Err(error) if is_unsupported_socket_readback(&error) => { @@ -524,8 +541,7 @@ mod tests { } Err(error) => panic!("{error}"), }; - assert_eq!(traffic_class & 0xfc, 184); - assert_eq!(traffic_class & 0b11, 0); + assert_eq!(traffic_class, 184); clear_dscp_on_socket(&socket, remote).unwrap(); assert_eq!(socket_traffic_class(&socket, remote).unwrap(), 0); diff --git a/crates/irtt-client/tests/protocol_options.rs b/crates/irtt-client/tests/protocol_options.rs index 83be36f..18b987b 100644 --- a/crates/irtt-client/tests/protocol_options.rs +++ b/crates/irtt-client/tests/protocol_options.rs @@ -163,7 +163,8 @@ fn hmac_rich_mode_uses_negotiated_echo_layout_and_decodes_reply() { fn strict_open_rejects_changed_compatibility_params_from_server() { let mut requested = default_params(); requested.length = 256; - requested.dscp = 46; + // Raw wire TOS/Traffic Class byte for DSCP codepoint 46 (EF). + requested.dscp = 184; requested.server_fill = server_fill("rand"); for returned in changed_compatibility_params(&requested) { @@ -183,7 +184,8 @@ fn strict_open_rejects_changed_compatibility_params_from_server() { fn loose_open_uses_returned_params_for_echo_layout_and_reply_parsing() { let mut requested = default_params(); requested.length = 128; - requested.dscp = 46; + // Raw wire TOS/Traffic Class byte for DSCP codepoint 46 (EF). + requested.dscp = 184; requested.server_fill = server_fill("rand"); let mut returned = requested.clone(); diff --git a/crates/irtt-client/tests/support/mod.rs b/crates/irtt-client/tests/support/mod.rs index 985f0cc..77f1487 100644 --- a/crates/irtt-client/tests/support/mod.rs +++ b/crates/irtt-client/tests/support/mod.rs @@ -158,7 +158,22 @@ pub fn params_for_modes(received_stats: ReceivedStats, stamp_at: StampAt, clock: } } +/// Builds a [`ClientConfig`] whose negotiated `Params` will match `params`. +/// +/// `params.dscp` is the raw IP TOS / Traffic Class byte, while +/// [`ClientConfig::dscp`] is the six-bit codepoint that produces it; shift +/// right by two to recover the codepoint the resulting config must request. +/// Panics if `params.dscp` has any ECN bits set, since those cannot be +/// recovered from a codepoint and this helper's contract would silently stop +/// holding otherwise. pub fn config_for_params(addr: SocketAddr, params: &Params) -> ClientConfig { + let traffic_class = + u8::try_from(params.dscp).expect("test DSCP must fit the raw traffic-class byte"); + assert_eq!( + traffic_class & 0b11, + 0, + "test Params::dscp must be representable by ClientConfig's DSCP codepoint" + ); ClientConfig { server_addr: addr.to_string(), duration: if params.duration_ns == 0 { @@ -175,7 +190,7 @@ pub fn config_for_params(addr: SocketAddr, params: &Params) -> ClientConfig { received_stats: params.received_stats, stamp_at: params.stamp_at, clock: params.clock, - dscp: u8::try_from(params.dscp).unwrap(), + dscp: traffic_class >> 2, server_fill: params.server_fill.as_ref().map(|fill| fill.value.clone()), open_timeouts: vec![Duration::from_millis(200)], socket_config: SocketConfig { diff --git a/crates/irtt-proto/src/params.rs b/crates/irtt-proto/src/params.rs index d6c8903..86ddcb3 100644 --- a/crates/irtt-proto/src/params.rs +++ b/crates/irtt-proto/src/params.rs @@ -20,6 +20,11 @@ pub struct Params { pub received_stats: ReceivedStats, pub stamp_at: StampAt, pub clock: Clock, + /// Raw IP TOS / Traffic Class byte (`0..=255`), not a six-bit DSCP + /// codepoint. A codepoint occupies the upper six bits of this byte; + /// callers that accept a codepoint from users or configuration must shift + /// it left by two before assigning it here. `encode`/`decode` carry this + /// value as-is and never apply that shift themselves. pub dscp: i64, pub server_fill: Option, } @@ -298,7 +303,11 @@ mod tests { } #[test] - fn params_round_trip_dscp_codepoints_without_shifting() { + fn params_round_trip_dscp_values_are_encoded_without_shifting() { + // `Params::dscp` is a raw wire byte; encode/decode carry whatever + // value is set without interpreting or shifting it, regardless of + // whether it happens to look like a codepoint (46) or a raw TOS byte + // (184). for dscp in [0, 46, 63, 64, 184, -1] { let params = Params { protocol_version: 1, @@ -316,11 +325,11 @@ mod tests { let encoded = params.encode(); assert!( encoded.windows(2).any(|bytes| bytes == [8, 92]), - "DSCP 46 must be encoded as param value 46" + "dscp value 46 must be encoded as param value 46" ); assert!( !encoded.windows(3).any(|bytes| bytes == [8, 0xf0, 0x02]), - "DSCP 46 must not be shifted to TOS byte 184 in Params encoding" + "Params::encode must not shift dscp 46 to 184; any codepoint shift is a caller concern" ); assert_eq!(Params::decode(&encoded).unwrap().dscp, 46); }