Skip to content

[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
wants to merge 26 commits into
base: dev
Choose a base branch
from

Conversation

Dimi1010
Copy link
Collaborator

@Dimi1010 Dimi1010 commented Jun 5, 2025

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 accepted const uint8_t* as a pointer to the buffer and immediately stripped the const in the constructor. This violates const correctness. The new constructors accept uint8_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's setRawData relied on the value of flag m_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 calling delete on stack allocated data.

The PR adds an enum RawPacketBufferPolicy to explicitly specify the type of ownership relation a RawPacket 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 and RawPacketBase

The inheritance hierarchy of raw packets has been expanded. MBufRawPacket no longer inherits from RawPacket and instead inherits from RawPacketBase as it was previously made to work around RawPacket's methods. The new interface IRawPacket 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 between RawPacket and MBufRawPacket, 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 of appendData and insertData. The added field m_RawDataCapacity tracks the buffer capacity independently of the used buffer data m_RawDataLen. The change allows RawPacket to track its internal buffer and fail operations that would result a buffer overflow without compromising the memory integrity.

The methods appendData and insertData's return values have been changed to size_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 of insertData(atIndex, nullptr, blockLen). The latter method's public documentation implies to expect a valid data buffer, so allowing nullptr 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.

Dimi1010 added 9 commits June 5, 2025 12:56
- 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.
@Dimi1010 Dimi1010 added refactoring API breaking change Pull request contains a breaking change to the public interface. API deprecation Pull requests that deprecate parts of the public interface. labels Jun 5, 2025
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant