Pkt ptrs/v39 #10796
Pkt ptrs/v39 #10796
Conversation
To match function naming style.
Improve readability by setting up data/data_len once before passing on to the other decoders. Work in preparation of other decoder changes.
In preparation of future Packet structure changes.
In preparation of making them union members.
Take less space in the TCPVars for tracking if SACKOK is set. Reduces size by 16 bytes.
No longer use a pointer, but rather an offset.
Addresses are pulled from embedded IPv4 header directly. Embeded hlen was unused.
Remove unused L4 header pointers.
Not used, so no need to keep them.
Only used in tests. For the tests, switch to getting headers from embedded IPv6 header.
Was only set, never checked.
Use a flag to indicate a calculated csum is available. Allows packet reset to just use memset.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10796 +/- ##
==========================================
+ Coverage 82.73% 82.79% +0.06%
==========================================
Files 927 926 -1
Lines 247785 247691 -94
==========================================
+ Hits 204996 205076 +80
+ Misses 42789 42615 -174
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 19989 |
|
Is there a redmine ticket for this ? |
There was a problem hiding this comment.
Thanks for the work Victor :-)
- CI : 🟢
- Code : checking now
- Commits segmentation : looks ok
- Commit messages : do we need tickets ?
One line withFor the tests, switch to getting headers from embedded IPv6 header.is a bit too long and may be its own and only line. - Git ID set : looks fine for me
- CLA : you already contributed :-p
- Doc update : not needed
- Redmine ticket : 🟠 to do ?
- Rustfmt : not needed
- Tests : how do you compute the size of the Packet struct ?
- Dependencies added: none
Why ? Code readability ? |
Bad commit title: this creates a rapport, but |
| } \ | ||
| } while(0) | ||
| #define DEBUG_VALIDATE_PACKET(p) \ | ||
| do { \ |
There was a problem hiding this comment.
Why did you add a do while wrapper ?
There was a problem hiding this comment.
many macro's have it, but this commit didn't add it
Not a good title, would prefer something like |
Yeah, mostly. Plus the type checking. But the many and often nested macro calls have often made code hard to follow. |
Just added https://redmine.openinfosecfoundation.org/issues/6938 |
| ip_hdr_offset = frag->ip_hdr_offset; | ||
| ip_hdr_offset = tracker->ip_hdr_offset; | ||
|
|
||
| /* This is the start of the fragmentable portion of the | ||
| * first packet. All fragment offsets are relative to | ||
| * this. */ | ||
| fragmentable_offset = frag->ip_hdr_offset + frag->hlen; | ||
| fragmentable_offset = tracker->ip_hdr_offset + frag->hlen; |
There was a problem hiding this comment.
Need to look closer, but could this be a possible behaviour change not covered by a unit test? 2 overlapping packets with offset 0. Instead of looking at the packet for each fragment, this will look at what was last set in the tracker.
|
Comments addressed in #10926. |
Redo much of the header pointer logic, cleaning up macro's and unifying various pointers that are mutually exclusive into unions.
Reduces size of the Packet struct from almost 600 bytes to around 450 bytes.
Includes #10762
Replaces #10791