feat: Extend Frame::Padding with length (#1762)

* feat: Extend `Frame::Padding` with a `length` field

This is preparation of qlog supporting the logging of runs of padding
frames via `payload_length`, instead of each one individually.

* Use `dv` more

* Fix test

* Address code review

* Add TODO

neqo-transport/src/connection/dump.rs

@@ -38,7 +38,8 @@ pub fn dump_packet(
             s.push_str(" [broken]...");
             break;
         };
-        if let Some(x) = f.dump() {
+        let x = f.dump();
+        if !x.is_empty() {
             write!(&mut s, "\n  {} {}", dir, &x).unwrap();
         }
     }

neqo-transport/src/connection/mod.rs

@@ -461,7 +461,7 @@ impl Connection {
     }
 
     /// # Errors
-    /// When the operation fails.    
+    /// When the operation fails.
     pub fn client_enable_ech(&mut self, ech_config_list: impl AsRef<[u8]>) -> Res<()> {
         self.crypto.client_enable_ech(ech_config_list)
     }
@@ -1560,24 +1560,8 @@ impl Connection {
         let mut ack_eliciting = false;
         let mut probing = true;
         let mut d = Decoder::from(&packet[..]);
-        let mut consecutive_padding = 0;
         while d.remaining() > 0 {
-            let mut f = Frame::decode(&mut d)?;
-
-            // Skip padding
-            while f == Frame::Padding && d.remaining() > 0 {
-                consecutive_padding += 1;
-                f = Frame::decode(&mut d)?;
-            }
-            if consecutive_padding > 0 {
-                qdebug!(
-                    [self],
-                    "PADDING frame repeated {} times",
-                    consecutive_padding
-                );
-                consecutive_padding = 0;
-            }
-
+            let f = Frame::decode(&mut d)?;
             ack_eliciting |= f.ack_eliciting();
             probing &= f.path_probing();
             let t = f.get_type();
@@ -2694,9 +2678,8 @@ impl Connection {
                 .input_frame(&frame, &mut self.stats.borrow_mut().frame_rx);
         }
         match frame {
-            Frame::Padding => {
-                // Note: This counts contiguous padding as a single frame.
-                self.stats.borrow_mut().frame_rx.padding += 1;
+            Frame::Padding(length) => {
+                self.stats.borrow_mut().frame_rx.padding += usize::from(length);
             }
             Frame::Ping => {
                 // If we get a PING and there are outstanding CRYPTO frames,

neqo-transport/src/connection/tests/handshake.rs

@@ -458,7 +458,7 @@ fn coalesce_05rtt() {
     assert_eq!(client.stats().dropped_rx, 0); // No Initial padding.
     assert_eq!(client.stats().packets_rx, 4);
     assert_eq!(client.stats().saved_datagrams, 1);
-    assert_eq!(client.stats().frame_rx.padding, 1); // Padding uses frames.
+    assert!(client.stats().frame_rx.padding > 0); // Padding uses frames.
 
     // Allow the handshake to complete.
     now += RTT / 2;

neqo-transport/src/frame.rs

@@ -20,7 +20,7 @@ use crate::{
 #[allow(clippy::module_name_repetitions)]
 pub type FrameType = u64;
 
-const FRAME_TYPE_PADDING: FrameType = 0x0;
+pub const FRAME_TYPE_PADDING: FrameType = 0x0;
 pub const FRAME_TYPE_PING: FrameType = 0x1;
 pub const FRAME_TYPE_ACK: FrameType = 0x2;
 const FRAME_TYPE_ACK_ECN: FrameType = 0x3;
@@ -103,7 +103,7 @@ pub struct AckRange {
 
 #[derive(PartialEq, Eq, Debug, Clone)]
 pub enum Frame<'a> {
-    Padding,
+    Padding(u16),
     Ping,
     Ack {
         largest_acknowledged: u64,
@@ -215,7 +215,7 @@ impl<'a> Frame<'a> {
 
     pub fn get_type(&self) -> FrameType {
         match self {
-            Self::Padding => FRAME_TYPE_PADDING,
+            Self::Padding { .. } => FRAME_TYPE_PADDING,
             Self::Ping => FRAME_TYPE_PING,
             Self::Ack { .. } => FRAME_TYPE_ACK, // We don't do ACK ECN.
             Self::ResetStream { .. } => FRAME_TYPE_RESET_STREAM,
@@ -288,7 +288,7 @@ impl<'a> Frame<'a> {
     pub fn ack_eliciting(&self) -> bool {
         !matches!(
             self,
-            Self::Ack { .. } | Self::Padding | Self::ConnectionClose { .. }
+            Self::Ack { .. } | Self::Padding { .. } | Self::ConnectionClose { .. }
         )
     }
 
@@ -297,7 +297,7 @@ impl<'a> Frame<'a> {
     pub fn path_probing(&self) -> bool {
         matches!(
             self,
-            Self::Padding
+            Self::Padding { .. }
                 | Self::NewConnectionId { .. }
                 | Self::PathChallenge { .. }
                 | Self::PathResponse { .. }
@@ -347,36 +347,34 @@ impl<'a> Frame<'a> {
         Ok(acked_ranges)
     }
 
-    pub fn dump(&self) -> Option<String> {
+    pub fn dump(&self) -> String {
         match self {
-            Self::Crypto { offset, data } => Some(format!(
-                "Crypto {{ offset: {}, len: {} }}",
-                offset,
-                data.len()
-            )),
+            Self::Crypto { offset, data } => {
+                format!("Crypto {{ offset: {}, len: {} }}", offset, data.len())
+            }
             Self::Stream {
                 stream_id,
                 offset,
                 fill,
                 data,
                 fin,
-            } => Some(format!(
+            } => format!(
                 "Stream {{ stream_id: {}, offset: {}, len: {}{}, fin: {} }}",
                 stream_id.as_u64(),
                 offset,
                 if *fill { ">>" } else { "" },
                 data.len(),
                 fin,
-            )),
-            Self::Padding => None,
-            Self::Datagram { data, .. } => Some(format!("Datagram {{ len: {} }}", data.len())),
-            _ => Some(format!("{self:?}")),
+            ),
+            Self::Padding(length) => format!("Padding {{ len: {length} }}"),
+            Self::Datagram { data, .. } => format!("Datagram {{ len: {} }}", data.len()),
+            _ => format!("{self:?}"),
         }
     }
 
     pub fn is_allowed(&self, pt: PacketType) -> bool {
         match self {
-            Self::Padding | Self::Ping => true,
+            Self::Padding { .. } | Self::Ping => true,
             Self::Crypto { .. }
             | Self::Ack { .. }
             | Self::ConnectionClose {
@@ -409,13 +407,23 @@ impl<'a> Frame<'a> {
         }
 
         // TODO(ekr@rtfm.com): check for minimal encoding
-        let t = d(dec.decode_varint())?;
+        let t = dv(dec)?;
         match t {
-            FRAME_TYPE_PADDING => Ok(Self::Padding),
+            FRAME_TYPE_PADDING => {
+                let mut length: u16 = 1;
+                while let Some(b) = dec.peek_byte() {
+                    if u64::from(b) != FRAME_TYPE_PADDING {
+                        break;
+                    }
+                    length += 1;
+                    dec.skip(1);
+                }
+                Ok(Self::Padding(length))
+            }
             FRAME_TYPE_PING => Ok(Self::Ping),
             FRAME_TYPE_RESET_STREAM => Ok(Self::ResetStream {
                 stream_id: StreamId::from(dv(dec)?),
-                application_error_code: d(dec.decode_varint())?,
+                application_error_code: dv(dec)?,
                 final_size: match dec.decode_varint() {
                     Some(v) => v,
                     _ => return Err(Error::NoMoreData),
@@ -457,7 +465,7 @@ impl<'a> Frame<'a> {
             }
             FRAME_TYPE_STOP_SENDING => Ok(Self::StopSending {
                 stream_id: StreamId::from(dv(dec)?),
-                application_error_code: d(dec.decode_varint())?,
+                application_error_code: dv(dec)?,
             }),
             FRAME_TYPE_CRYPTO => {
                 let offset = dv(dec)?;
@@ -563,7 +571,7 @@ impl<'a> Frame<'a> {
                 Ok(Self::PathResponse { data: datav })
             }
             FRAME_TYPE_CONNECTION_CLOSE_TRANSPORT | FRAME_TYPE_CONNECTION_CLOSE_APPLICATION => {
-                let error_code = CloseError::from_type_bit(t, d(dec.decode_varint())?);
+                let error_code = CloseError::from_type_bit(t, dv(dec)?);
                 let frame_type = if t == FRAME_TYPE_CONNECTION_CLOSE_TRANSPORT {
                     dv(dec)?
                 } else {
@@ -631,8 +639,10 @@ mod tests {
 
     #[test]
     fn padding() {
-        let f = Frame::Padding;
+        let f = Frame::Padding(1);
         just_dec(&f, "00");
+        let f = Frame::Padding(2);
+        just_dec(&f, "0000");
     }
 
     #[test]
@@ -888,8 +898,8 @@ mod tests {
 
     #[test]
     fn test_compare() {
-        let f1 = Frame::Padding;
-        let f2 = Frame::Padding;
+        let f1 = Frame::Padding(1);
+        let f2 = Frame::Padding(1);
         let f3 = Frame::Crypto {
             offset: 0,
             data: &[1, 2, 3],

neqo-transport/src/packet/mod.rs

@@ -18,6 +18,7 @@ use neqo_crypto::random;
 use crate::{
     cid::{ConnectionId, ConnectionIdDecoder, ConnectionIdRef, MAX_CONNECTION_ID_LEN},
     crypto::{CryptoDxState, CryptoSpace, CryptoStates},
+    frame::FRAME_TYPE_PADDING,
     version::{Version, WireVersion},
     Error, Res,
 };
@@ -257,7 +258,8 @@ impl PacketBuilder {
     /// Returns true if padding was added.
     pub fn pad(&mut self) -> bool {
         if self.padding && !self.is_long() {
-            self.encoder.pad_to(self.limit, 0);
+            self.encoder
+                .pad_to(self.limit, FRAME_TYPE_PADDING.try_into().unwrap());
             true
         } else {
             false