PR feedback

microsoft · Jan 24, 2025 · 1c2656c · 1c2656c
1 parent 30d5e68
commit 1c2656c
Show file tree

Hide file tree

Showing 2 changed files with 14 additions and 21 deletions.
diff --git a/src/core/recv_buffer.c b/src/core/recv_buffer.c
@@ -52,7 +52,7 @@ void
 QuicRecvChunkInitialize(
     _Inout_ QUIC_RECV_CHUNK* Chunk,
     _In_ uint32_t AllocLength,
-    _In_ uint8_t* Buffer
+    _Inout_updates_(AllocLength) uint8_t* Buffer
     )
 {
     Chunk->AllocLength = AllocLength;
@@ -76,8 +76,6 @@ QuicRecvBufferInitialize(
     CXPLAT_DBG_ASSERT(VirtualBufferLength != 0 && (VirtualBufferLength & (VirtualBufferLength - 1)) == 0); // Power of 2
     CXPLAT_DBG_ASSERT(AllocBufferLength <= VirtualBufferLength);
 
-    // TODO guhetier: What to do when starting in the EXTERNAL mode?
-    //     Can the code handle not having any chunks? In practice we should never start directly in EXTERNAL
     QUIC_RECV_CHUNK* Chunk = NULL;
     if (PreallocatedChunk != NULL) {
         RecvBuffer->PreallocatedChunk = PreallocatedChunk;
@@ -199,17 +197,15 @@ QuicRecvBufferProvideChunks(
 {
     // TODO guhetier: Consider making this valid in external mode only
     //  and have a reset function to change the mode.
-    // But that mean having to detect if the buffer is not empty + dealing with
-    // no chunks on init?
     CXPLAT_DBG_ASSERT(!CxPlatListIsEmpty(Chunks));
 
     //
     // External chunks can be provided only if already in external mode, or if
     // nothing has been written to the buffer yet.
     //
-    uint64_t rangeMax;
+    uint64_t RangeMax;
     if (RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_EXTERNAL &&
-        (QuicRangeGetMaxSafe(&RecvBuffer->WrittenRanges, &rangeMax) && rangeMax > 0)) {
+        (QuicRangeGetMaxSafe(&RecvBuffer->WrittenRanges, &RangeMax) && RangeMax > 0)) {
         return QUIC_STATUS_INVALID_STATE;
     }
 
@@ -685,7 +681,7 @@ QuicRecvBufferWrite(
     // N.B. We do this before updating the written ranges below so we don't have
     // to support rolling back those changes on the possible allocation failure
     // here.
-    // This is skiped in external mode since the entire virtual length is
+    // This is skipped in external mode since the entire virtual length is
     // always allocated.
     //
     if (RecvBuffer->RecvMode != QUIC_RECV_BUF_MODE_EXTERNAL) {
@@ -949,8 +945,7 @@ QuicRecvBufferRead(
         //
         // Continue reading from the next chunks until we run out of buffers or data.
         //
-        while (*BufferCount < ProvidedBufferCount && remainingDataToRead > 0)
-        {
+        while (*BufferCount < ProvidedBufferCount && remainingDataToRead > 0) {
             Chunk =
                 CXPLAT_CONTAINING_RECORD(
                     Chunk->Link.Flink,
@@ -1116,8 +1111,7 @@ QuicRecvBufferFullDrain(
         CXPLAT_FRE_ASSERTMSG(DrainLength == 0, "App drained more than was available!");
         CXPLAT_DBG_ASSERT(RecvBuffer->ReadLength == 0);
 
-        if (RecvBuffer->RecvMode == QUIC_RECV_BUF_MODE_EXTERNAL)
-        {
+        if (RecvBuffer->RecvMode == QUIC_RECV_BUF_MODE_EXTERNAL) {
             //
             // In EXTERNAL mode, external chunks are never re-used:
             // free the last chunk.

diff --git a/src/core/recv_buffer.h b/src/core/recv_buffer.h
@@ -13,29 +13,28 @@ typedef enum QUIC_RECV_BUF_MODE {
     QUIC_RECV_BUF_MODE_SINGLE,      // Only one receive with a single contiguous buffer at a time.
     QUIC_RECV_BUF_MODE_CIRCULAR,    // Only one receive that may indicate two contiguous buffers at a time.
     QUIC_RECV_BUF_MODE_MULTIPLE,    // Multiple independent receives that may indicate up to two contiguous buffers at a time.
-    // TODO guhetier: improve documentation
     QUIC_RECV_BUF_MODE_EXTERNAL     // Uses memory buffer provided by the app. Only one receive at a time,
-                                    // that may indicate up to the number of provided buffers.
+                                    //     that may indicate up to the number of provided buffers.
 } QUIC_RECV_BUF_MODE;
 
 //
 // Represents a single contiguous range of bytes.
 //
 typedef struct QUIC_RECV_CHUNK {
-    CXPLAT_LIST_ENTRY Link;         // Link in the list of chunks.
-    uint32_t AllocLength : 31;      // Allocation size of Buffer
-    uint32_t ExternalReference : 1; // Indicates the buffer is being used externally.
-    uint8_t *Buffer;                // Pointer to the buffer itself. Doesn't need to be freed independently:
-                                    //     - for internally allocated buffers, points in the same allocation.
-                                    //     - for exteral buffers, the buffer isn't owned
+    CXPLAT_LIST_ENTRY Link;                     // Link in the list of chunks.
+    uint32_t AllocLength : 31;                  // Allocation size of Buffer
+    uint32_t ExternalReference : 1;             // Indicates the buffer is being used externally.
+    _Field_size_(AllocLength) uint8_t *Buffer;  // Pointer to the buffer itself. Doesn't need to be freed independently:
+                                                //     - for internally allocated buffers, points in the same allocation.
+                                                //     - for exteral buffers, the buffer isn't owned
 } QUIC_RECV_CHUNK;
 
 _IRQL_requires_max_(DISPATCH_LEVEL)
 void
 QuicRecvChunkInitialize(
     _Inout_ QUIC_RECV_CHUNK* Chunk,
     _In_ uint32_t AllocLength,
-    _In_ uint8_t* Buffer
+    _Inout_updates_(AllocLength) uint8_t* Buffer
     );
 
 typedef struct QUIC_RECV_BUFFER {