feat: Make reason_phrase a String (#1862)

* feat: Make `reason_phrase` a String To make the debug logs a bit easier to parse. * Fix clippy * Update neqo-transport/src/frame.rs Co-authored-by: Martin Thomson <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Update neqo-transport/src/qlog.rs Co-authored-by: Max Inden <[email protected]> Signed-off-by: Lars Eggert <[email protected]> * Address code review --------- Signed-off-by: Lars Eggert <[email protected]> Co-authored-by: Martin Thomson <[email protected]> Co-authored-by: Max Inden <[email protected]>
mozilla · May 2, 2024 · 620244d · 620244d
1 parent 8192300
commit 620244d
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 29 deletions.
diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs
@@ -2828,7 +2828,6 @@ impl Connection {
                 reason_phrase,
             } => {
                 self.stats.borrow_mut().frame_rx.connection_close += 1;
-                let reason_phrase = String::from_utf8_lossy(&reason_phrase);
                 qinfo!(
                     [self],
                     "ConnectionClose received. Error code: {:?} frame type {:x} reason {}",

diff --git a/neqo-transport/src/frame.rs b/neqo-transport/src/frame.rs
@@ -184,7 +184,7 @@ pub enum Frame<'a> {
         frame_type: u64,
         // Not a reference as we use this to hold the value.
         // This is not used in optimized builds anyway.
-        reason_phrase: Vec<u8>,
+        reason_phrase: String,
     },
     HandshakeDone,
     AckFrequency {
@@ -614,7 +614,7 @@ impl<'a> Frame<'a> {
                     0
                 };
                 // We can tolerate this copy for now.
-                let reason_phrase = d(dec.decode_vvec())?.to_vec();
+                let reason_phrase = String::from_utf8_lossy(d(dec.decode_vvec())?).to_string();
                 Ok(Self::ConnectionClose {
                     error_code,
                     frame_type,
@@ -925,7 +925,7 @@ mod tests {
         let f = Frame::ConnectionClose {
             error_code: CloseError::Transport(0x5678),
             frame_type: 0x1234,
-            reason_phrase: vec![0x01, 0x02, 0x03],
+            reason_phrase: String::from("\x01\x02\x03"),
         };
 
         just_dec(&f, "1c80005678523403010203");
@@ -936,7 +936,7 @@ mod tests {
         let f = Frame::ConnectionClose {
             error_code: CloseError::Application(0x5678),
             frame_type: 0,
-            reason_phrase: vec![0x01, 0x02, 0x03],
+            reason_phrase: String::from("\x01\x02\x03"),
         };
 
         just_dec(&f, "1d8000567803010203");

diff --git a/neqo-transport/src/qlog.rs b/neqo-transport/src/qlog.rs
@@ -205,7 +205,7 @@ pub fn packet_sent(
         let mut frames = SmallVec::new();
         while d.remaining() > 0 {
             if let Ok(f) = Frame::decode(&mut d) {
-                frames.push(QuicFrame::from(&f));
+                frames.push(QuicFrame::from(f));
             } else {
                 qinfo!("qlog: invalid frame");
                 break;
@@ -293,7 +293,7 @@ pub fn packet_received(
 
         while d.remaining() > 0 {
             if let Ok(f) = Frame::decode(&mut d) {
-                frames.push(QuicFrame::from(&f));
+                frames.push(QuicFrame::from(f));
             } else {
                 qinfo!("qlog: invalid frame");
                 break;
@@ -387,12 +387,12 @@ pub fn metrics_updated(qlog: &mut NeqoQlog, updated_metrics: &[QlogMetric]) {
 
 #[allow(clippy::too_many_lines)] // Yeah, but it's a nice match.
 #[allow(clippy::cast_possible_truncation, clippy::cast_precision_loss)] // No choice here.
-impl From<&Frame<'_>> for QuicFrame {
-    fn from(frame: &Frame) -> Self {
+impl From<Frame<'_>> for QuicFrame {
+    fn from(frame: Frame) -> Self {
         match frame {
             Frame::Padding(len) => QuicFrame::Padding {
                 length: None,
-                payload_length: u32::from(*len),
+                payload_length: u32::from(len),
             },
             Frame::Ping => QuicFrame::Ping {
                 length: None,
@@ -406,7 +406,7 @@ impl From<&Frame<'_>> for QuicFrame {
                 ecn_count,
             } => {
                 let ranges =
-                    Frame::decode_ack_frame(*largest_acknowledged, *first_ack_range, ack_ranges)
+                    Frame::decode_ack_frame(largest_acknowledged, first_ack_range, &ack_ranges)
                         .ok();
 
                 let acked_ranges = ranges.map(|all| {
@@ -418,7 +418,7 @@ impl From<&Frame<'_>> for QuicFrame {
                 });
 
                 QuicFrame::Ack {
-                    ack_delay: Some(*ack_delay as f32 / 1000.0),
+                    ack_delay: Some(ack_delay as f32 / 1000.0),
                     acked_ranges,
                     ect1: ecn_count.map(|c| c[IpTosEcn::Ect1]),
                     ect0: ecn_count.map(|c| c[IpTosEcn::Ect0]),
@@ -433,8 +433,8 @@ impl From<&Frame<'_>> for QuicFrame {
                 final_size,
             } => QuicFrame::ResetStream {
                 stream_id: stream_id.as_u64(),
-                error_code: *application_error_code,
-                final_size: *final_size,
+                error_code: application_error_code,
+                final_size,
                 length: None,
                 payload_length: None,
             },
@@ -443,12 +443,12 @@ impl From<&Frame<'_>> for QuicFrame {
                 application_error_code,
             } => QuicFrame::StopSending {
                 stream_id: stream_id.as_u64(),
-                error_code: *application_error_code,
+                error_code: application_error_code,
                 length: None,
                 payload_length: None,
             },
             Frame::Crypto { offset, data } => QuicFrame::Crypto {
-                offset: *offset,
+                offset,
                 length: data.len() as u64,
             },
             Frame::NewToken { token } => QuicFrame::NewToken {
@@ -470,20 +470,20 @@ impl From<&Frame<'_>> for QuicFrame {
                 ..
             } => QuicFrame::Stream {
                 stream_id: stream_id.as_u64(),
-                offset: *offset,
+                offset,
                 length: data.len() as u64,
-                fin: Some(*fin),
+                fin: Some(fin),
                 raw: None,
             },
             Frame::MaxData { maximum_data } => QuicFrame::MaxData {
-                maximum: *maximum_data,
+                maximum: maximum_data,
             },
             Frame::MaxStreamData {
                 stream_id,
                 maximum_stream_data,
             } => QuicFrame::MaxStreamData {
                 stream_id: stream_id.as_u64(),
-                maximum: *maximum_stream_data,
+                maximum: maximum_stream_data,
             },
             Frame::MaxStreams {
                 stream_type,
@@ -493,15 +493,15 @@ impl From<&Frame<'_>> for QuicFrame {
                     NeqoStreamType::BiDi => StreamType::Bidirectional,
                     NeqoStreamType::UniDi => StreamType::Unidirectional,
                 },
-                maximum: *maximum_streams,
+                maximum: maximum_streams,
             },
-            Frame::DataBlocked { data_limit } => QuicFrame::DataBlocked { limit: *data_limit },
+            Frame::DataBlocked { data_limit } => QuicFrame::DataBlocked { limit: data_limit },
             Frame::StreamDataBlocked {
                 stream_id,
                 stream_data_limit,
             } => QuicFrame::StreamDataBlocked {
                 stream_id: stream_id.as_u64(),
-                limit: *stream_data_limit,
+                limit: stream_data_limit,
             },
             Frame::StreamsBlocked {
                 stream_type,
@@ -511,22 +511,22 @@ impl From<&Frame<'_>> for QuicFrame {
                     NeqoStreamType::BiDi => StreamType::Bidirectional,
                     NeqoStreamType::UniDi => StreamType::Unidirectional,
                 },
-                limit: *stream_limit,
+                limit: stream_limit,
             },
             Frame::NewConnectionId {
                 sequence_number,
                 retire_prior,
                 connection_id,
                 stateless_reset_token,
             } => QuicFrame::NewConnectionId {
-                sequence_number: *sequence_number as u32,
-                retire_prior_to: *retire_prior as u32,
+                sequence_number: sequence_number as u32,
+                retire_prior_to: retire_prior as u32,
                 connection_id_length: Some(connection_id.len() as u8),
                 connection_id: hex(connection_id),
                 stateless_reset_token: Some(hex(stateless_reset_token)),
             },
             Frame::RetireConnectionId { sequence_number } => QuicFrame::RetireConnectionId {
-                sequence_number: *sequence_number as u32,
+                sequence_number: sequence_number as u32,
             },
             Frame::PathChallenge { data } => QuicFrame::PathChallenge {
                 data: Some(hex(data)),
@@ -545,8 +545,8 @@ impl From<&Frame<'_>> for QuicFrame {
                 },
                 error_code: Some(error_code.code()),
                 error_code_value: Some(0),
-                reason: Some(String::from_utf8_lossy(reason_phrase).to_string()),
-                trigger_frame_type: Some(*frame_type),
+                reason: Some(reason_phrase),
+                trigger_frame_type: Some(frame_type),
             },
             Frame::HandshakeDone => QuicFrame::HandshakeDone,
             Frame::AckFrequency { .. } => QuicFrame::Unknown {