Fix frame length #324
Fix frame length #324
Conversation
src/pdu.cpp
Outdated
| const PDU* ptr(inner_pdu_); | ||
| while (ptr) { | ||
| sz += ptr->advertised_size(); | ||
| if(ptr->pdu_type() != IP) { |
There was a problem hiding this comment.
This check shouldn't be here. This has to work no matter what PDUs a packet contains. I think a better and generic solution to this is to have the default implementation return header_size() + trailer_size() + inner_pdu()->size() (this last one obviously only if there's an inner PDU). This way you're defining the advertised PDU size to be the size of the current PDU + whatever the next PDU tells you. So if the next PDU is IP, then it will simply return its total length without going further down the chain.
There was a problem hiding this comment.
Sorry, I mean inner_pdu()->advertised_size() here.
There was a problem hiding this comment.
Well the IP::tot_len() contains the length of all inner PDUs does it not?
That was my understanding after looking at the IP header in wireshark.
There was a problem hiding this comment.
Exactly, which is why IP can get away with just returning tot_len as its advertised size vs the rest of the PDUs which need to keep going down the PDU chain to find out.
There was a problem hiding this comment.
I see the check is no longer needed, since IP::advertised_size() will be the recursion anchor.
I'll fix it.
There was a problem hiding this comment.
Made the changes, just waiting for your continuous integration.
include/tins/ip.h
Outdated
| @@ -281,6 +281,11 @@ class TINS_API IP : public PDU { | |||
|
|
|||
| /* Getters */ | |||
|
|
|||
| uint32_t advertised_size() const { | |||
| uint32_t sz = static_cast<uint32_t>(tot_len()); | |||
There was a problem hiding this comment.
Let's just make this a single return, no need for an intermediate variable.
There was a problem hiding this comment.
Made the changes, just waiting for your continuous integration.
pepper-jk
force-pushed
the
fix_frame_length
branch
from
74e2a00 to
67c36ce
Compare
src/pdu.cpp
Outdated
| @@ -85,6 +85,10 @@ uint32_t PDU::size() const { | |||
| return sz; | |||
| } | |||
|
|
|||
| uint32_t PDU::advertised_size() const { | |||
| return header_size() + trailer_size() + inner_pdu()->advertised_size(); | |||
There was a problem hiding this comment.
As I mentioned in my comment, this should first check if there's an inner_pdu, otherwise this will blow up.
pepper-jk
force-pushed
the
fix_frame_length
branch
from
67c36ce to
dc702f4
Compare
|
Is there anything else that needs changing? |
|
Nope, it looks good. I just didn't get to merge this. Thanks! |
|
Cool, thanks for merging. |
|
I could probably release a patch version soon. |
|
Any updates on the patch version? |
|
Oops, I completely forgot. I'll definitely do this tonight! Sorry about that |
|
I just pushed v4.2. Sorry for the delay! |
This pull request should fix issue #300.
I implemented a virtual method
PDU::advertised_size(), which defaults to the sum ofheader_size() + trailer_size()plusadvertised_size()of all inner PDUs down to the IP PDU.The
IP::advertised_size()method returns thetot_leninstead.The PacketWriter now uses this virtual method to determine the frame length before writing the packet out.
Please let me know if this solution is to your liking.