Improve encapsulation of logic relating to Layer attachment to Packet. #2004
Conversation
… to a Layer's ownership from regular fields.
| // Should this really always delete m_Data? What if the layer is attached to a packet? | ||
| if (m_Data != nullptr) | ||
| delete[] m_Data; |
There was a problem hiding this comment.
See comment? This would cause an error if called on a Layer that does not own its data span.
| else | ||
| { | ||
| layer->m_Packet = nullptr; | ||
| layer->m_AllocationInfo.detach(); |
There was a problem hiding this comment.
This line in particular created a subtle bug in the old implementation.
If a layer is originally owned by the packet instance, and detached, the m_IsAllocatedInPacket flag was not zeroed. If the layer has then been attached to another Packet. The attached layer was considered as if managed by the new packet even if attached with addLayer(layer, false) which should not transfer ownership of the layer object.
| /// | ||
| /// If 'false', the Layer object is considered unmanaged and the user is responsible for freeing it. | ||
| /// This is commonly the case for layers created on the stack and attached to a Packet. | ||
| bool managedByPacket = false; |
There was a problem hiding this comment.
Maybe ownedByPacket is a more descriptive name? 🤔
| PTF_ASSERT_TRUE(packetWithoutTunnel.addLayer(vxlanEthLayer, true)); | ||
| PTF_ASSERT_TRUE(packetWithoutTunnel.addLayer(vxlanIP4Layer, true)); | ||
| PTF_ASSERT_TRUE(packetWithoutTunnel.addLayer(vxlanIcmpLayer, true)); |
There was a problem hiding this comment.
Or should we keep the test as is, but correctly delete the layers afterwards?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #2004 +/- ##
==========================================
+ Coverage 83.41% 83.45% +0.03%
==========================================
Files 311 311
Lines 55019 54576 -443
Branches 11816 11806 -10
==========================================
- Hits 45892 45544 -348
- Misses 7852 8199 +347
+ Partials 1275 833 -442
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
This PR intended to improve the encapsulation and clarity of Layer field data. As per the C++ core guidelines
protecteddata fields are generally undesired as they represent difficulty in maintaining base class invariants across derived classes.The main object of this PR is the introduction of a new helper struct
LayerAllocationInfoin the internal namespace, to hold allocation information for Layer objects and provide behavior helper methods. The helper structure replaces is contained as a field in aLayerto represent its current allocation state.LayerAllocationInfoand chosen fields.The information structure contains 2 members:
Packet* attachedPacket, replacingm_Packetpointer insideLayer.bool managedByPacket, replacingm_isAllocatedInPacketflag insideLayer.The replacements were chosen due to their relevancy to the allocation behaviour of
Layerand little else. Those fields were previously intermixed with other fields in theLayerobject causing confusion when determining their use and scope.attachedPacket(previouslym_Packet) is exclusively used for indication if the layer's data is owned by a packet and to forward requests for structural data changes to the underlying packet buffer, if available. The new name was chosen to better represent that use.managedByPacket(previouslym_IsAllocatedInPacket) is exclusively used as flag to determine if theLayerobject's lifecycle is managed by thePacketinstance it is attached to. The field intricately tied to the state ofattachedPacketand as such is included to the helper structure.The new fields include extensive documentation regarding their use, improving the maintainability of the code. ( Think about the you in 5 months reading it 🙂 )
Behaviour methods instead of direct field mutation
The new helper structure adds two behavior methods:
attachPacket(Packet* packet, bool managed, bool force = false): This method encapsulates the behavior of attaching a layer to a Packet instance. It allows aPacketinstance attaching aLayerto itself to update the information in one step instead of manually having to directly updateprivatefields to theLayerinstance. The method also provides integrated sanity checks if aLayeris being attached to aPacketinstance twice.detach(): Encapsulates the behaviour of detaching aLayerfromPacket. It allows aPacketinstance detaching aLayerto inform it of its new state in one step.Protected data accessors and private fields
The new
LayerAllocationInfostructure is held as a private field insideLayer. The change improves data integrity as the allocation information (packet or managed flag) is unavailable for accidental modification by derived classes.To keep existing behaviour of a
Layerinstance forwarding its attachment to any other instance it constructs during parsing, a new protected accessorgetAttachedPacket()has been added and instances of direct use ofm_Packetare replaced by it.Bug Fixes
m_IsAllocatedInPacketflag when detaching a layer. If said layer is attached to a new packet instance, the old implementation disregarded the ownership flag and considered the layer instance as owned by the packet, due to the layer flag being set from the previous packet.