Fix #4651: Improve SOMEIP.fragment()#4655
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4655 +/- ##
==========================================
+ Coverage 81.53% 82.12% +0.58%
==========================================
Files 359 359
Lines 86565 86583 +18
==========================================
+ Hits 70582 71106 +524
+ Misses 15983 15477 -506
|
|
Aw, conflicts :/ |
cb37b59 to
775d58e
Compare
fixed |
gpotter2
left a comment
There was a problem hiding this comment.
Reading only this code and not the spec, I kinda feel like "data" could have been a Padding payload. It all in all seems a bit complicated.
If your fix fixes the issue, it's fine to merge it though.
|
The current implementation "tries" to do two things which are somewhat in conflict with each other.
To support multiple packets in one frame (1), #4632 was merged. This uses bind_layers of SOMEIP Custom payload classes are supported by an explicit data field for which the callback can be overloaded dynamically: By moving the data field in the padding payload, we would need to call I would say, the current solution is more "straight forward". Do you see an alternative way of doing this? |
Co-authored-by: gpotter2 <10530980+gpotter2@users.noreply.github.com>
Revert suggested change
* Fix secdev#4651: Improve SOMEIP.fragment() * fix flake
Usually, payloads are stored inside
SOMEIP.data[0]. In the past I made this become a PacketListField, so that customizations for custom data interpretations can be added to theSOMEIP.get_payload_cls_by_srv_id().However, this needs to be respected in the
fragmentfunction. This PR adds support for this custom handling. Also, there is still the possibility to useadd_payload, which would create aSOMEIP/Rawpacket. Therefore this changes will determine, if the payload is outside or inside theSOMEIPpacket.