From 1c2656c8ce3208aa087ba8143fa6cdcf4b1506cc Mon Sep 17 00:00:00 2001 From: Guillaume Hetier Date: Fri, 24 Jan 2025 15:41:00 -0800 Subject: [PATCH] PR feedback --- src/core/recv_buffer.c | 18 ++++++------------ src/core/recv_buffer.h | 17 ++++++++--------- 2 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/core/recv_buffer.c b/src/core/recv_buffer.c index 0b7805824a..69a83a5dc1 100644 --- 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 index 8ab3039baa..38053e581f 100644 --- a/src/core/recv_buffer.h +++ b/src/core/recv_buffer.h @@ -13,21 +13,20 @@ 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) @@ -35,7 +34,7 @@ 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 {