Move simple field access to accessor functions

This helps encapsulation by ensuring that no outside users can mutate
these fields.  Reducing the amount of logic that needs to be reasoned
about.

Author: flub

quinn-proto/src/connection/mod.rs

@@ -519,7 +519,7 @@ impl Connection {
             // always spill into the next datagram.
             let pn = self.packet_number_filter.peek(&self.spaces[SpaceId::Data]);
             let frame_space_1rtt = buf
-                .segment_size
+                .segment_size()
                 .saturating_sub(self.predict_1rtt_overhead(Some(pn)));
 
             // Is there data or a close message to send in this space?
@@ -558,11 +558,11 @@ impl Connection {
             // We are NOT coalescing (the default is we are, so this was turned off in an
             // earlier iteration) OR there is not enough space for another *packet* in this
             // datagram (buf_capacity - buf_end == unused space in datagram).
-            if !coalesce || buf.buf_capacity - buf_end < MIN_PACKET_SPACE + tag_len {
+            if !coalesce || buf.datagram_max_offset() - buf_end < MIN_PACKET_SPACE + tag_len {
                 // We need to send 1 more datagram and extend the buffer for that.
 
                 // Is 1 more datagram allowed?
-                if buf.buf_capacity >= buf.segment_size * buf.max_datagrams {
+                if buf.datagram_max_offset() >= buf.segment_size() * buf.max_datagrams {
                     // No more datagrams allowed
                     break;
                 }
@@ -573,10 +573,9 @@ impl Connection {
                 // for starting another datagram. If there is any anti-amplification
                 // budget left, we always allow a full MTU to be sent
                 // (see https://github.com/quinn-rs/quinn/issues/1082)
-                if self
-                    .path
-                    .anti_amplification_blocked((buf.segment_size * buf.num_datagrams) as u64 + 1)
-                {
+                if self.path.anti_amplification_blocked(
+                    (buf.segment_size() * buf.num_datagrams()) as u64 + 1,
+                ) {
                     trace!("blocked by anti-amplification");
                     break;
                 }
@@ -586,13 +585,13 @@ impl Connection {
                 if ack_eliciting && self.spaces[space_id].loss_probes == 0 {
                     // Assume the current packet will get padded to fill the segment
                     let untracked_bytes = if let Some(builder) = &builder_storage {
-                        buf.buf_capacity - builder.partial_encode.start
+                        buf.datagram_max_offset() - builder.partial_encode.start
                     } else {
                         0
                     } as u64;
-                    debug_assert!(untracked_bytes <= buf.segment_size as u64);
+                    debug_assert!(untracked_bytes <= buf.segment_size() as u64);
 
-                    let bytes_to_send = buf.segment_size as u64 + untracked_bytes;
+                    let bytes_to_send = buf.segment_size() as u64 + untracked_bytes;
                     if self.path.in_flight.bytes + bytes_to_send >= self.path.congestion.window() {
                         space_idx += 1;
                         congestion_blocked = true;
@@ -626,7 +625,7 @@ impl Connection {
                         builder.pad_to(MIN_INITIAL_SIZE);
                     }
 
-                    if buf.num_datagrams > 1 {
+                    if buf.num_datagrams() > 1 {
                         // If too many padding bytes would be required to continue the GSO batch
                         // after this packet, end the GSO batch here. Ensures that fixed-size frames
                         // with heterogeneous sizes (e.g. application datagrams) won't inadvertently
@@ -641,27 +640,28 @@ impl Connection {
                         // `buf_capacity` by less than `segment_size`.
                         const MAX_PADDING: usize = 16;
                         let packet_len_unpadded = cmp::max(builder.min_size, buf.len())
-                            - buf.datagram_start
+                            - buf.datagram_start_offset()
                             + builder.tag_len;
-                        if packet_len_unpadded + MAX_PADDING < buf.segment_size
-                            || buf.datagram_start + buf.segment_size > buf.buf_capacity
+                        if packet_len_unpadded + MAX_PADDING < buf.segment_size()
+                            || buf.datagram_start_offset() + buf.segment_size()
+                                > buf.datagram_max_offset()
                         {
                             trace!(
                                 "GSO truncated by demand for {} padding bytes or loss probe",
-                                buf.segment_size - packet_len_unpadded
+                                buf.segment_size() - packet_len_unpadded
                             );
                             builder_storage = Some(builder);
                             break;
                         }
 
                         // Pad the current datagram to GSO segment size so it can be included in the
                         // GSO batch.
-                        builder.pad_to(buf.segment_size as u16);
+                        builder.pad_to(buf.segment_size() as u16);
                     }
 
                     builder.finish_and_track(now, self, sent_frames.take(), buf.buf);
 
-                    if buf.num_datagrams == 1 && space_id == SpaceId::Data {
+                    if buf.num_datagrams() == 1 && space_id == SpaceId::Data {
                         // Now that we know the size of the first datagram, check whether
                         // the data we planned to send will fit in the next segment.  If
                         // not, bails out and leave it for the next GSO batch.  We can't
@@ -673,7 +673,7 @@ impl Connection {
                         buf.clip_datagram_size();
 
                         let frame_space_1rtt = buf
-                            .segment_size
+                            .segment_size()
                             .saturating_sub(self.predict_1rtt_overhead(Some(pn)));
                         if self.space_can_send(space_id, frame_space_1rtt).is_empty() {
                             break;
@@ -691,7 +691,7 @@ impl Connection {
                         // unexpectedly.
                         buf.start_new_datagram_with_size(std::cmp::min(
                             usize::from(INITIAL_MTU),
-                            buf.segment_size,
+                            buf.segment_size(),
                         ));
                     }
                 };
@@ -706,7 +706,7 @@ impl Connection {
                 }
             }
 
-            debug_assert!(buf.buf_capacity - buf.len() >= MIN_PACKET_SPACE);
+            debug_assert!(buf.datagram_max_offset() - buf.len() >= MIN_PACKET_SPACE);
 
             //
             // From here on, we've determined that a packet will definitely be sent.
@@ -809,7 +809,7 @@ impl Connection {
 
             // Send an off-path PATH_RESPONSE. Prioritized over on-path data to ensure that path
             // validation can occur while the link is saturated.
-            if space_id == SpaceId::Data && buf.num_datagrams == 1 {
+            if space_id == SpaceId::Data && buf.num_datagrams() == 1 {
                 if let Some((token, remote)) = self.path_responses.pop_off_path(self.path.remote) {
                     // `unwrap` guaranteed to succeed because `builder_storage` was populated just
                     // above.
@@ -857,7 +857,7 @@ impl Connection {
                 !(sent.is_ack_only(&self.streams)
                     && !can_send.acks
                     && can_send.other
-                    && (buf.buf_capacity - builder.datagram_start)
+                    && (buf.datagram_max_offset() - builder.datagram_start)
                         == self.path.current_mtu() as usize
                     && self.datagrams.outgoing.is_empty()),
                 "SendableFrames was {can_send:?}, but only ACKs have been written"
@@ -898,10 +898,10 @@ impl Connection {
                 .mtud
                 .poll_transmit(now, self.packet_number_filter.peek(&self.spaces[space_id]))?;
 
-            debug_assert_eq!(buf.num_datagrams, 0);
+            debug_assert_eq!(buf.num_datagrams(), 0);
             buf.start_new_datagram_with_size(probe_size as usize);
 
-            debug_assert_eq!(buf.datagram_start, 0);
+            debug_assert_eq!(buf.datagram_start_offset(), 0);
             let mut builder =
                 PacketBuilder::new(now, space_id, self.rem_cids.active(), &mut buf, true, self)?;
 
@@ -934,13 +934,13 @@ impl Connection {
         trace!(
             "sending {} bytes in {} datagrams",
             buf.len(),
-            buf.num_datagrams
+            buf.num_datagrams()
         );
         self.path.total_sent = self.path.total_sent.saturating_add(buf.len() as u64);
 
         self.stats
             .udp_tx
-            .on_sent(buf.num_datagrams as u64, buf.len());
+            .on_sent(buf.num_datagrams() as u64, buf.len());
 
         Some(Transmit {
             destination: self.path.remote,
@@ -950,9 +950,9 @@ impl Connection {
             } else {
                 None
             },
-            segment_size: match buf.num_datagrams {
+            segment_size: match buf.num_datagrams() {
                 1 => None,
-                _ => Some(buf.segment_size),
+                _ => Some(buf.segment_size()),
             },
             src_ip: self.local_ip,
         })
@@ -981,7 +981,7 @@ impl Connection {
         // sent once, immediately after migration, when the CID is known to be valid. Even
         // if a post-migration packet caused the CID to be retired, it's fair to pretend
         // this is sent first.
-        debug_assert_eq!(buf.datagram_start, 0);
+        debug_assert_eq!(buf.datagram_start_offset(), 0);
         let mut builder = PacketBuilder::new(now, SpaceId::Data, *prev_cid, buf, false, self)?;
         trace!("validating previous path with PATH_CHALLENGE {:08x}", token);
         buf.write(frame::FrameType::PATH_CHALLENGE);

quinn-proto/src/connection/packet_builder.rs

@@ -149,11 +149,11 @@ impl PacketBuilder {
             buffer.len() + (sample_size + 4).saturating_sub(number.len() + tag_len),
             partial_encode.start + dst_cid.len() + 6,
         );
-        let max_size = buffer.buf_capacity - tag_len;
+        let max_size = buffer.datagram_max_offset() - tag_len;
         debug_assert!(max_size >= min_size);
 
         Some(Self {
-            datagram_start: buffer.datagram_start,
+            datagram_start: buffer.datagram_start_offset(),
             space: space_id,
             partial_encode,
             exact_number,

quinn-proto/src/connection/transmit_buf.rs

@@ -30,12 +30,12 @@ pub(super) struct TransmitBuf<'a> {
     ///
     /// Note that when coalescing packets this might be before the start of the current
     /// packet.
-    pub(super) datagram_start: usize,
+    datagram_start: usize,
     /// The maximum offset allowed to be used for the current datagram in the buffer
     ///
     /// The first and last datagram in a batch are allowed to be smaller then the maximum
     /// size.  All datagrams in between need to be exactly this size.
-    pub(super) buf_capacity: usize,
+    buf_capacity: usize,
     /// The maximum number of datagrams allowed to write into [`TransmitBuf::buf`]
     pub(super) max_datagrams: usize,
     /// The number of datagrams already (partially) written into the buffer
@@ -50,7 +50,7 @@ pub(super) struct TransmitBuf<'a> {
     /// For the first datagram this is set to the maximum size a datagram is allowed to be:
     /// the current path MTU.  After the first datagram is finished this is reduced to the
     /// size of the first datagram and can no longer change.
-    pub(super) segment_size: usize,
+    segment_size: usize,
 }
 
 impl<'a> TransmitBuf<'a> {
@@ -139,6 +139,37 @@ impl<'a> TransmitBuf<'a> {
         self.segment_size = self.buf.len();
     }
 
+    /// Returns the GSO segment size
+    ///
+    /// This is also the maximum size datagrams are allowed to be.  The first and last
+    /// datagram in a batch are allowed to be smaller however.  After the first datagram the
+    /// segment size is clipped to the size of the first datagram.
+    pub(super) fn segment_size(&self) -> usize {
+        self.segment_size
+    }
+
+    /// Returns the number of datagrams written into the buffer
+    ///
+    /// The last datagram is not necessarily finished yet.
+    pub(super) fn num_datagrams(&self) -> usize {
+        self.num_datagrams
+    }
+
+    /// Returns the start offset of the current datagram in the buffer
+    ///
+    /// In other words, this offset contains the first byte of the current datagram.
+    pub(super) fn datagram_start_offset(&self) -> usize {
+        self.datagram_start
+    }
+
+    /// Returns the maximum offset in the buffer allowed for the current datagram
+    ///
+    /// The first and last datagram in a batch are allowed to be smaller then the maximum
+    /// size.  All datagrams in between need to be exactly this size.
+    pub(super) fn datagram_max_offset(&self) -> usize {
+        self.buf_capacity
+    }
+
     /// Returns `true` if the buffer did not have anything written into it
     pub(super) fn is_empty(&self) -> bool {
         self.len() == 0