-
Notifications
You must be signed in to change notification settings - Fork 715
Description
This issue is a general overview of infrastructure and API enhancements related to RawPacket
mostly explored in #1845 .
Separation of RawPacket
and MBufRawPacket
Currently MBufRawPacket
inherits from RawPacket
mainly to be interchangeable with it in the Layer
infrastructure. The two packet classes however, differ significantly in their internal mechanics.
RawPacket
focuses on heap allocated or externally managed memory.MBufRawPacket
focuses on packets in DPDK MBuf managed memory.
It is my opinion that need for MBufRawPacket
to inherit from RawPacket
causes issues with the classes' maintenance. Notable cases are RawPacket
's copy operations, which need to work both for RawPacket
and for MBufRawPacket
.
My proposal for resolution of the issue is the addition of a pure interface class IRawPacket
that provides a common interface and that both classes inherit it. RawPacket
can keep its existing functionality intact without the need to handle cases where it is derived from, while MBufRawPacket
is detached from the base implementation allowing greater freedom of implementation.
The existing Packet
infrastructure can be refactored to utilize IRawPacket*
where RawPacket*
was previously used, with minimal changes to the internals.
Improvements of const correctness of RawPacket
constructor parameters.
In the current implementation the RawPacket
constructor accepts a uint8_t const*
data buffer pointer. This is incorrect as the pointer is immediately stripped of its const
qualifier. It is my opinion that that constructor should be deprecated and an overload that accepts a non-const qualified data buffer pointer should be provided instead.
This change forces the const
stripping to be done outside of the class, ensuring that the user makes a conscious decision to ignore const-correctness and pass a buffer that should not be modified.
RawPacket
internal capacity tracking
In the current implementation RawPacket
does not track the capacity of the buffer it owns / references and relies on external users to track its capacity for it. This is mentioned in the descriptions of RawPacket::appendData
and RawPacket::insertData
, warning of memory corruption if invalid input is provided.
I propose that the buffer capacity is tracked internally, as that would allow better memory safety during append or insert operations. It would also allow better control over the buffer depending on its ownership.
Improving clarity of buffer ownership
The current way of controlling buffer ownership of RawPacket
is during construction by the flag deleteRawDataAtDestructor
. If the flag is true
the buffer is deallocated on RawPacket
destruction.
This leaves RawPacket::setRawData
ambiguous on its ownership transfer. The method does not provide such a parameter and inherits the value from the constructed buffer. This means that a RawPacket
that has been constructed by a non-owning buffer, cannot ever have setRawData
with an owning buffer and vice versa.
My proposal is the addition of RawPacketBufferPolicy
enum that controls the ownership transfer. The enum is to be a required parameter on every method that replaces the internal buffer of RawPacket
.
The buffer policy can be one of 4 values:
- Copy: The
RawPacket
allocates a new buffer and copies the input buffer into it. - Move: The
RawPacket
assumes ownership of the input buffer. - StrictReference: The
RawPacket
references the input buffer without assuming ownership. Operations that will exceed the buffer's capacity will fail and raise errors. The input buffer must remain valid for the duration it is in use. - SoftReference: The
RawPacket
references the input buffer without assuming ownership. Operations that will exceed the buffer's capacity will cause a reallocation to an internal buffer. The input buffer must remain valid for the duration it is in use.
The following policies will allow flexibility in the desired behaviour of RawPacket
.
- Externally managed buffers for read only situations can use
StrictReference
orSoftReference
to skip allocating and coping the buffer. - Externally managed buffers for read/write situations can use
StrictReference
to enforce usage of the provided buffer. - General use cases where simplicity is preferred can use
Copy
,Move
orSoftReference
to let theRawPacket
handle memory management.
Improvements of RawPacket::insertData
The current insertData
method has an undocumented use case of packet.insertData(index, nullptr, bufferLen)
in which an uninitialized buffer is inserted in the packet.
In the interest of code clarity, I propose that a new method insertUninitializedData(index, bufferLen)
is added to assume this function, while insertData
is modified to require a non-null data pointer to be provided.