-
Notifications
You must be signed in to change notification settings - Fork 715
[WIP] General refactor of RawPacket
to remove ambiguity in API and streamline logic.
#1845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
Dimi1010
wants to merge
26
commits into
seladb:dev
Choose a base branch
from
Dimi1010:refactor/raw-packet
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Added IRawPacket as a pure interface. - Added RawPacketBase as a common abstract class that defines methods for timestamp and link layer fields. - Refactored RawPacket to be final class. It is to handle only standard memory. - Added `RawPacketBufferPolicy` to explicitly control transfer of buffer ownerships and relations.
…of buffer capacity indicator and taking const uint8_t* and stripping constness under the hood.
…cific to each instance. Added copy of RawPacketSet flag.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
API breaking change
Pull request contains a breaking change to the public interface.
API deprecation
Pull requests that deprecate parts of the public interface.
refactoring
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a general refactor of
RawPacket
to fix some core design issues with the class.Due to the fact that it is a core library change, it will probably need to be merged prior to a major revision version.
Fixed const correctness of input buffer parameter
Previously
RawPacket
acceptedconst uint8_t*
as a pointer to the buffer and immediately stripped the const in the constructor. This violates const correctness. The new constructors acceptuint8_t*
as a pointer to the buffer to require the user to knowingly strip const ness if usage of const buffer is required.Explicit buffer policies
Previously,
RawPacket
'ssetRawData
relied on the value of flagm_DeleteRawDataAtDestructor
to choose if the raw packet owned the input buffer or just referenced it. This ambiguity can lead to critical errors as users have no way to query the value after construction, leading to memory leaks, dangling pointers, or UB by callingdelete
on stack allocated data.The PR adds an enum
RawPacketBufferPolicy
to explicitly specify the type of ownership relation aRawPacket
object will have with its underlying buffer on every operation that replaces it. The users can choose from one of 4 policies (Copy, Move, SoftReference, StrictReference). The latter two policies utilize the input buffer without assuming ownership, but SoftReference allows transfer of the packet data to an internal buffer if resize is required, while strict reference will refuse reallocation requests.New base classes:
IRawPacket
andRawPacketBase
The inheritance hierarchy of raw packets has been expanded.
MBufRawPacket
no longer inherits fromRawPacket
and instead inherits fromRawPacketBase
as it was previously made to work aroundRawPacket
's methods. The new interfaceIRawPacket
is supposed to provide common methods through which higher level abstractions (Packet
) can interact with the raw data buffers.The class
RawPacketBase
is to provide common functionality betweenRawPacket
andMBufRawPacket
, namely timestamp and link layer fields.Enhanced buffer capacity tracking in
RawPacket
The PR adds the capability for
RawPacket
to track its internal (or external) buffer capacity. Previously, the object relied on users to correctly know the buffer capacity via external means, allowing the possibility of buffer overflow via improper usage ofappendData
andinsertData
. The added fieldm_RawDataCapacity
tracks the buffer capacity independently of the used buffer datam_RawDataLen
. The change allowsRawPacket
to track its internal buffer and fail operations that would result a buffer overflow without compromising the memory integrity.The methods
appendData
andinsertData
's return values have been changed tosize_t
to report success or error and the number of bytes written.Added separate API call for inserting block for overwrite in the middle of a packet.
Added a new method
insertUninitializedData
that replaces the previous usage ofinsertData(atIndex, nullptr, blockLen)
. The latter method's public documentation implies to expect a valid data buffer, so allowingnullptr
as a valid buffer value and extending the buffer with uninitialized data violates the principle of least surprise.The old signature
insertData(atIndex, nullptr, blockLen)
is now an error.