|
|
Created:
12 years, 9 months ago by Tommaso Pecorella Modified:
12 years, 9 months ago CC:
ns-3-reviews_googlegroups.com Visibility:
Public. |
Patch Set 1 : Initial proposal #
Total comments: 4
Patch Set 2 : Fixed according to Tom's comments #
Total comments: 6
Patch Set 3 : Fix or/and and includes #Patch Set 4 : Added a warning about possible packet overflow #
Total comments: 2
Patch Set 5 : Fixed calc on possible packet overflow #MessagesTotal messages: 30
Consider this as a possible patch to fix bug # 1102
Sign in to reply to this message.
I would also suggest that you change this in the Print() method: - << "offset " << m_fragmentOffset << " " + << "offset (bytes) " << m_fragmentOffset << " " http://codereview.appspot.com/4608045/diff/2001/src/internet/model/ipv4-heade... File src/internet/model/ipv4-header.cc (right): http://codereview.appspot.com/4608045/diff/2001/src/internet/model/ipv4-heade... src/internet/model/ipv4-header.cc:121: // check if the user is trying to set an arbitrary offset s/arbitrary/invalid http://codereview.appspot.com/4608045/diff/2001/src/internet/model/ipv4-heade... src/internet/model/ipv4-header.cc:122: NS_ASSERT (offsetBytes | 0x7); I would suggest to use instead NS_ABORT_MSG_IF ((offsetBytes | 0x7), "offsetBytes must be multiple of 8 bytes"); since this is a case of runtime invalid input from user and should be enforced in optimized builds. (where to use NS_ASSERT vs NS_ABORT/NS_FATAL_ERROR could be topic for another "best practices" discussion) http://codereview.appspot.com/4608045/diff/2001/src/internet/model/ipv4-heade... src/internet/model/ipv4-header.cc:128: NS_ASSERT (m_fragmentOffset | 0x7); Not sure why an assert is needed in a getter, suggest to remove. http://codereview.appspot.com/4608045/diff/2001/src/internet/model/ipv4-header.h File src/internet/model/ipv4-header.h (right): http://codereview.appspot.com/4608045/diff/2001/src/internet/model/ipv4-heade... src/internet/model/ipv4-header.h:75: * Hence, the function does assume that the offset is a multiple of 8. s/does assume/does enforce
Sign in to reply to this message.
Sign in to reply to this message.
On 2011/06/15 22:09:05, Tommaso Pecorella wrote: Sorry if I push for it, but we need one more reviewer and Tom's comments on this.
Sign in to reply to this message.
Some more comments http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-heade... File src/internet/model/ipv4-header.cc (right): http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-heade... src/internet/model/ipv4-header.cc:120: NS_ABORT_MSG_IF ((offsetBytes | 0x7), "offsetBytes must be multiple of 8 bytes"); 1. Did you mean to write (offsetBytes & 0x7) ? 2. how can we ensure the frag offset will require only a IPv4 packet <= 65535b? (((fragment offset)*8) - 1 + Ipv4 hdr size) > 65535 (Max IPV4 packet size).
Sign in to reply to this message.
comment on Doxygen http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-header.h File src/internet/model/ipv4-header.h (right): http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-heade... src/internet/model/ipv4-header.h:116: * \returns the offset of this fragment measured in bytes from the start. If you enforced the user to provide an offset that is a multiple of 8 in "SetFragmentOffset", then GetFragmentOffset should say "returns the offset of this fragment in units of 8 byte blocks from the start?
Sign in to reply to this message.
http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-heade... File src/internet/model/ipv4-header.cc (right): http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-heade... src/internet/model/ipv4-header.cc:120: NS_ABORT_MSG_IF ((offsetBytes | 0x7), "offsetBytes must be multiple of 8 bytes"); On 2011/06/20 20:53:20, johnjoe7 wrote: > 1. Did you mean to write (offsetBytes & 0x7) ? Yes. > 2. how can we ensure the frag offset will require only a IPv4 packet <= 65535b? > > (((fragment > offset)*8) - 1 + Ipv4 hdr size) > 65535 (Max IPV4 packet size). We don't. But the fragment offset is including the header. http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-header.h File src/internet/model/ipv4-header.h (right): http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-heade... src/internet/model/ipv4-header.h:116: * \returns the offset of this fragment measured in bytes from the start. On 2011/06/20 21:01:09, johnjoe7 wrote: > If you enforced the user to provide an offset that is a multiple of 8 in > "SetFragmentOffset", > then GetFragmentOffset should say "returns the offset of this fragment in units > of 8 byte blocks from the start? Nope, the offset returned is in bytes from the start. The user should be aware that is a multiple of 8 bytes anyway. Documenting is fine. Over-documenting isn't.
Sign in to reply to this message.
http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-header.h File src/internet/model/ipv4-header.h (right): http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-heade... src/internet/model/ipv4-header.h:116: * \returns the offset of this fragment measured in bytes from the start. On 2011/06/20 22:42:31, Tommaso Pecorella wrote: > On 2011/06/20 21:01:09, johnjoe7 wrote: > > If you enforced the user to provide an offset that is a multiple of 8 in > > "SetFragmentOffset", > > then GetFragmentOffset should say "returns the offset of this fragment in > units > > of 8 byte blocks from the start? > > Nope, the offset returned is in bytes from the start. The user should be aware > that is a multiple of 8 bytes anyway. Documenting is fine. Over-documenting > isn't. Well, others were not in favor of implying 8b blocks in the function signature (Get8bblockFragOffset), neither do we want to document that the function returns 8b blocks? How is the end-user of a public API supposed to know this?
Sign in to reply to this message.
http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-heade... File src/internet/model/ipv4-header.cc (right): http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-heade... src/internet/model/ipv4-header.cc:120: NS_ABORT_MSG_IF ((offsetBytes | 0x7), "offsetBytes must be multiple of 8 bytes"); On 2011/06/20 22:42:31, Tommaso Pecorella wrote: > On 2011/06/20 20:53:20, johnjoe7 wrote: > > 1. Did you mean to write (offsetBytes & 0x7) ? > > Yes. > > > 2. how can we ensure the frag offset will require only a IPv4 packet <= > 65535b? > > > > (((fragment > > offset)*8) - 1 + Ipv4 hdr size) > 65535 (Max IPV4 packet size). > > We don't. But the fragment offset is including the header. I don't understand, how is the fragment offset included in the header size before serialization? A simple ASSERT ((fragoffset * 8)-1) + 20 > 65535) may address this condition .For example fragoffset >=8190 won't fit within a legal IP header.
Sign in to reply to this message.
On 2011/06/20 23:01:21, johnjoe7 wrote: > http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-heade... > File src/internet/model/ipv4-header.cc (right): > > http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-heade... > src/internet/model/ipv4-header.cc:120: NS_ABORT_MSG_IF ((offsetBytes | 0x7), > "offsetBytes must be multiple of 8 bytes"); > On 2011/06/20 22:42:31, Tommaso Pecorella wrote: > > On 2011/06/20 20:53:20, johnjoe7 wrote: > > > 1. Did you mean to write (offsetBytes & 0x7) ? > > > > Yes. > > > > > 2. how can we ensure the frag offset will require only a IPv4 packet <= > > 65535b? > > > > > > (((fragment > > > offset)*8) - 1 + Ipv4 hdr size) > 65535 (Max IPV4 packet size). > > > > We don't. But the fragment offset is including the header. > > I don't understand, how is the fragment offset included in the header size > before serialization? > > A simple ASSERT ((fragoffset * 8)-1) + 20 > 65535) may address this condition > .For example fragoffset >=8190 won't fit within a legal IP header. meant to say legal IP packet
Sign in to reply to this message.
Sign in to reply to this message.
I'm fine with this version. I would also be fine with adding an additional check on the offsetBytes that it does not exceed the value 65512 (John raised this point).
Sign in to reply to this message.
On 2011/06/21 13:36:00, Tom Henderson wrote: > I'm fine with this version. I would also be fine with adding an additional > check on the offsetBytes that it does not exceed the value 65512 (John raised > this point). Let me clarify a bit my points, maybe my answer was a bit too concise. Maybe it's me but I don't see why we should add that check. The offsetBytes max value is 65528 (0xFFF8) and that is a perfectly legitimate value. Of course this would mean that the **rebuilt** packet could be longer than 65535 bytes (due to the vary last fragment offset and fragment length) but that's a completely different kind of check and, in my opinion, it can not be addressed in this part of the code. Moreover it can not arise from anything but one case: fragmented jumbo ip packets. I've never seen any in my whole life tbh. To clarify my previous statement about the legit values, let's make some numbers. 1st fragment: offset 0 2nd: offset x (bytes, let's make it easy), meaning you have to strip the IP header and concatenate the payload starting from byte x 3rd and so on: same as 2nd. So if a fragment has the max allowable fragment offset (0x1FFF) its payload will be placed starting at byte 65528. This of course means that in order to make a valid IP packet, that fragment length must not be longer than 7 bytes + IP header length, but that's a completely different issue, as it involves checking not only the offset, but also the fragment length. It might be (and maybe is) a valid check for reassembly, but not for the "simple" fragment option setter. Cheers, Tommaso
Sign in to reply to this message.
On 2011/06/21 14:20:18, Tommaso Pecorella wrote: > On 2011/06/21 13:36:00, Tom Henderson wrote: > > I'm fine with this version. I would also be fine with adding an additional > > check on the offsetBytes that it does not exceed the value 65512 (John raised > > this point). > > Let me clarify a bit my points, maybe my answer was a bit too concise. > > Maybe it's me but I don't see why we should add that check. The offsetBytes max > value is 65528 (0xFFF8) and that is a perfectly legitimate value. Of course this > would mean that the **rebuilt** packet could be longer than 65535 bytes (due to > the vary last fragment offset and fragment length) but that's a completely > different kind of check and, in my opinion, it can not be addressed in this part > of the code. Moreover it can not arise from anything but one case: fragmented > jumbo ip packets. I've never seen any in my whole life tbh. > > To clarify my previous statement about the legit values, let's make some > numbers. > 1st fragment: offset 0 > 2nd: offset x (bytes, let's make it easy), meaning you have to strip the IP > header and concatenate the payload starting from byte x > 3rd and so on: same as 2nd. > > So if a fragment has the max allowable fragment offset (0x1FFF) its payload will > be placed starting at byte 65528. This of course means that in order to make a > valid IP packet, that fragment length must not be longer than 7 bytes + IP > header length, but that's a completely different issue, as it involves checking > not only the offset, but also the fragment length. > > It might be (and maybe is) a valid check for reassembly, but not for the > "simple" fragment option setter. > > Cheers, > > Tommaso Well we wanted to make this API usable by any layer besides the fragmentation module.Which means any layer that bypass the frag module needs to know about the rules/limits of IPv4 headers . i.e different modules will now have overlapping knowledge about the same thing. It may be a reassembly requirement , but in that sense, we don't need to ABORT_IF or check for multiples of 8 either. We could take in any number not a multiple of 8 ,fit it within 13 bits and send it over the network and let the reassembler deal with it. any Ipv4 fragment offset of 8190 (65520) is instantly illegal as 20b additional IPV4 header will never change , thus causing an overflow. Another check that needs to be done before sending to data-link layer, is variable length IPv4 options header which can cause a total ipv4 header length of upto 60 bytes. The ASSERT here is a debug aid to catch accidental coding since we want this API to be used by any layer (layers that can bypass the fragmentation module). If ASSERT is too harsh may be a WARN will do.The problem is originated by the sender and the sender has the knowledge to rectify it unless it is a stack custom-made to do a DOS attack (i.e make the receiver reassembler reassemble X fragments to realize the last fragment is bogus enough to drop the complete reassembly). Just my opinion
Sign in to reply to this message.
On 2011/06/21 14:20:18, Tommaso Pecorella wrote: > On 2011/06/21 13:36:00, Tom Henderson wrote: > > I'm fine with this version. I would also be fine with adding an additional > > check on the offsetBytes that it does not exceed the value 65512 (John raised > > this point). > > Let me clarify a bit my points, maybe my answer was a bit too concise. > > Maybe it's me but I don't see why we should add that check. I too am not certain about adding that check, which is why I am fine with it either way. Overflows of IP datagram total length can be checked elsewhere in the code. This seems to be a corner case of no real practical interest, while the other check (& 0x7) for multiple of 8 bytes can catch programming errors so I would recommend to keep that one.
Sign in to reply to this message.
On 2011/06/21 14:54:18, johnjoe7 wrote: > Well we wanted to make this API usable by any layer besides the fragmentation > module.Which means any layer that bypass the frag module needs to know about the > rules/limits of IPv4 headers . i.e different modules will now have overlapping > knowledge about the same thing. Well, I assume that is someone is writing something that interacts with this he/she should know what's going on. I assume as well that is someone is trying to write a reassembly code he/she can as well check the fragmentation counterpart, isn't it ? > It may be a reassembly requirement , but in that sense, we don't need to > ABORT_IF or check for multiples of 8 either. We could take in any number not a > multiple of 8 ,fit it within 13 bits and send it over the network and let the > reassembler deal with it. Not really, the multiple-of-8 is mandatory since the IP itself defines the fragment offset as a number representing multiple of 8 bytes offsets. So that check is mandatory, as later in the code the offset will have a ">>3". So if someone tries to set a fragment offset of, let's say, 252 bytes, there will be a problem for sure as upon receiving the fragment will have an offset of 248 bytes. That's why the assert is there. > any Ipv4 fragment offset of 8190 (65520) is instantly illegal as 20b additional > IPV4 header will never change , thus causing an overflow. > Another check that needs to be done before sending to data-link layer, is > variable length IPv4 options header which can cause a total ipv4 header length > of upto 60 bytes. Nope, as I said the offset is belonging to the payload. The IP header (and the IP optional headers) are removed before the reassembly. But that's not really important. > The ASSERT here is a debug aid to catch accidental coding since we want this API > to be used by any layer (layers that can bypass the fragmentation module). If > ASSERT is too harsh may be a WARN will do. A warning could be interesting, but it should check as well the other data. BTW, you have a point... BUT I found where the problem is. Wikipedia: "This allows a maximum offset of (2^{13} – 1) × 8 = 65,528 bytes which would exceed the maximum IP packet length of 65,535 bytes with the header length included (65,528 + 20 = 65,548 bytes)." This is an horribly wrong statement, as the IP header of the first fragment is already counted in the offset of the second, and from there onward the offsets are relative to the payload (not the header). Nevertheless, I added a warning.
Sign in to reply to this message.
Sign in to reply to this message.
On 2011/06/21 17:03:06, Tommaso Pecorella wrote: > This is an horribly wrong statement, as the IP header of the first fragment is > already counted in the offset of the second, and from there onward the offsets > are relative to the payload (not the header). > Nevertheless, I added a warning. I am not sure whether it is wrong. RFC 1122 implies that you must be able to do this: B. Before IP fragmentation or after IP reassembly: ______________________________________ | IP hdr | transport| Application Data | |________|____hdr___|__________________| <-------- Datagram ------------------> <-------- Message -----------> which would imply that you must be able to form a valid IP header, which limits in practice the size of the IP payload.
Sign in to reply to this message.
On 2011/06/21 17:03:06, Tommaso Pecorella wrote: > On 2011/06/21 14:54:18, johnjoe7 wrote: > > Well we wanted to make this API usable by any layer besides the fragmentation > > module.Which means any layer that bypass the frag module needs to know about > the > > rules/limits of IPv4 headers . i.e different modules will now have overlapping > > knowledge about the same thing. > > Well, I assume that is someone is writing something that interacts with this > he/she should know what's going on. I assume as well that is someone is trying > to write a reassembly code he/she can as well check the fragmentation > counterpart, isn't it ? > > > It may be a reassembly requirement , but in that sense, we don't need to > > ABORT_IF or check for multiples of 8 either. We could take in any number not a > > multiple of 8 ,fit it within 13 bits and send it over the network and let the > > reassembler deal with it. > > Not really, the multiple-of-8 is mandatory since the IP itself defines the > fragment offset as a number representing multiple of 8 bytes offsets. So that > check is mandatory, as later in the code the offset will have a ">>3". So if > someone tries to set a fragment offset of, let's say, 252 bytes, there will be a > problem for sure as upon receiving the fragment will have an offset of 248 > bytes. That's why the assert is there. > > > any Ipv4 fragment offset of 8190 (65520) is instantly illegal as 20b > additional > > IPV4 header will never change , thus causing an overflow. > > Another check that needs to be done before sending to data-link layer, is > > variable length IPv4 options header which can cause a total ipv4 header length > > of upto 60 bytes. > > Nope, as I said the offset is belonging to the payload. The IP header (and the > IP optional headers) are removed before the reassembly. But that's not really > important. > From RFC 791 This format allows 2**13 = 8192 fragments of 8 octets each for a total of 65,536 octets. Note that this is consistent with the the datagram total length field (of course, the header is counted in the total length and not in the fragments). > > The ASSERT here is a debug aid to catch accidental coding since we want this > API > > to be used by any layer (layers that can bypass the fragmentation module). If > > ASSERT is too harsh may be a WARN will do. > > A warning could be interesting, but it should check as well the other data. > > BTW, you have a point... BUT I found where the problem is. Wikipedia: > "This allows a maximum offset of (2^{13} – 1) × 8 = 65,528 bytes which would > exceed the maximum IP packet length of 65,535 bytes with the header length > included (65,528 + 20 = 65,548 bytes)." > This is an horribly wrong statement, as the IP header of the first fragment is > already counted in the offset of the second, and from there onward the offsets > are relative to the payload (not the header). > Nevertheless, I added a warning.
Sign in to reply to this message.
Total Length: 16 bits The total length of an IP datagram which includes IP hdr cannnot exceed 65536. From RFC 791 Fragmentation Procedure Maximum sized datagram that can be Tx'ed is the MTU If the total length is less than or equal the maximum transmission unit then submit this datagram to the next step in datagram processing; otherwise cut the datagram into two fragments, Note: The total length here is still referring to the Total length of the datagram. From RFC 1122 IP Datagram An IP datagram is the unit of end-to-end transmission in the IP protocol. An IP datagram consists of an IP header followed by transport layer data, i.e., of an IP header followed by a message. In the description of the internet layer (Section 3), the unqualified term "datagram" should be understood to refer to an IP datagram. Note: IP Datagram includes Ipv4 header. Back to RFC 791 Reassembly procedure. (10) IF MF = 0 THEN TDL <- TL-(IHL*4)+(FO*8) (11) IF FO = 0 THEN put header in header buffer (12) IF TDL # 0 (13) AND all RCVBT bits from 0 to (TDL+7)/8 are set (14) THEN TL <- TDL+(IHL*4) (15) Submit datagram to next step; (16) free all reassembly resources for this BUFID; DONE. Note: At the end of reassembly TDL is assigned to TL the total length of the datagram before proceeding to "next step". TDL is limited by fragment offset (F0) by the line TDL <- TL-(IHL*4)+(FO*8). F0 cannot exceed 8192 * 8 per RFC 791 This format allows 2**13 = 8192 fragments of 8 octets each for a total of 65,536 octets. Note that this is consistent with the the datagram total length field (of course, the header is counted in the total length and not in the fragments). This is how I have interpreted it.
Sign in to reply to this message.
Also note: although this issue is irrelevant now due to consensus. I just wanted to mention, I have not seen any implementation be it linux or other proprietary network devices which have their own Ipv4 stack need to check if fragment offset is a "multiple of 8".I maybe wrong, there may be some stacks out there. Typically because ">> 3" is far more efficient from a processor point-of-view (Shift right logical in MIPS) vs (ANDI with 0x07 and one branch instruction needed if (X & 0x07)) is not null. But I will go with the consensus on this as this topic was already discussed and settled.
Sign in to reply to this message.
Very interesting topic indeed, and it might be relevant on the fragmentation / reassembly (different code to be reviewed tho). About how it is implemented, I guess it's a matter about what's most efficient. Plus in the implementations there's no need to provide "accessible" APIs to the fragment offset as typically it's handled inside the IP stack, so it's an implementation matter if and how the offset is handled. Anyway, it ends up that I might be have a small bug in my other code, I'll double check with wireshark and the real linux implementation. But again, it doesn't really vary this part, so... formal feedback, shall I push this ? Cheers, Tommaso
Sign in to reply to this message.
On 2011/06/21 21:08:38, Tommaso Pecorella wrote: > Very interesting topic indeed, and it might be relevant on the fragmentation / > reassembly (different code to be reviewed tho). > > About how it is implemented, I guess it's a matter about what's most efficient. > Plus in the implementations there's no need to provide "accessible" APIs to the > fragment offset as typically it's handled inside the IP stack, so it's an > implementation matter if and how the offset is handled. > > Anyway, it ends up that I might be have a small bug in my other code, I'll > double check with wireshark and the real linux implementation. But again, it > doesn't really vary this part, so... formal feedback, shall I push this ? > > Cheers, > > Tommaso I will leave you and Tom to make the final call.
Sign in to reply to this message.
http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-head... File src/internet/model/ipv4-header.cc (right): http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-head... src/internet/model/ipv4-header.cc:128: { I did not follow the rationale for adding this warning here; I would assume this should just be a getter and the client code can decide whether it doesn't like the value returned.
Sign in to reply to this message.
http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-head... File src/internet/model/ipv4-header.cc (right): http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-head... src/internet/model/ipv4-header.cc:128: { On 2011/06/21 21:56:30, Tom Henderson wrote: > I did not follow the rationale for adding this warning here; I would assume this > should just be a getter and the client code can decide whether it doesn't like > the value returned. That was my tough at first, but John did raise the point that the reassembled packet could exceed the maximum packet size in some cases. To ease the debugging of "proprietary" reassembly code, the check was added. It should not slow down simulations (I don't see many cases where fragmentation should occur, after all we lived so far without), so it's harmless to check. I do agree that it could be somewhere else, but this is the "safest" place to put it. Here or in the deserialize. Shall I put it in the deserialize ?
Sign in to reply to this message.
On 2011/06/22 00:03:36, Tommaso Pecorella wrote: > http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-head... > File src/internet/model/ipv4-header.cc (right): > > http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-head... > src/internet/model/ipv4-header.cc:128: { > On 2011/06/21 21:56:30, Tom Henderson wrote: > > I did not follow the rationale for adding this warning here; I would assume > this > > should just be a getter and the client code can decide whether it doesn't like > > the value returned. > > That was my tough at first, but John did raise the point that the reassembled > packet could exceed the maximum packet size in some cases. > > To ease the debugging of "proprietary" reassembly code, the check was added. It > should not slow down simulations (I don't see many cases where fragmentation > should occur, after all we lived so far without), so it's harmless to check. > > I do agree that it could be somewhere else, but this is the "safest" place to > put it. Here or in the deserialize. Shall I put it in the deserialize ? I am OK with leaving it, it is just a LOG statement. Should it be instead (m_fragmentOffset + m_payloadSize + 5*4) to account for the header?
Sign in to reply to this message.
Sign in to reply to this message.
On 2011/06/22 05:08:26, Tom Henderson wrote: > On 2011/06/22 00:03:36, Tommaso Pecorella wrote: > > > http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-head... > > File src/internet/model/ipv4-header.cc (right): > > > > > http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-head... > > src/internet/model/ipv4-header.cc:128: { > > On 2011/06/21 21:56:30, Tom Henderson wrote: > > > I did not follow the rationale for adding this warning here; I would assume > > this > > > should just be a getter and the client code can decide whether it doesn't > like > > > the value returned. > > > > That was my tough at first, but John did raise the point that the reassembled > > packet could exceed the maximum packet size in some cases. > > > > To ease the debugging of "proprietary" reassembly code, the check was added. > It > > should not slow down simulations (I don't see many cases where fragmentation > > should occur, after all we lived so far without), so it's harmless to check. > > > > I do agree that it could be somewhere else, but this is the "safest" place to > > put it. Here or in the deserialize. Shall I put it in the deserialize ? > > I am OK with leaving it, it is just a LOG statement. Should it be instead > (m_fragmentOffset + m_payloadSize + 5*4) to account for the header? Yes, but only because we don't have IP header options support (yet). Otherwise the calc would be a bit more complex. Anyway, added 20 bytes. Tommaso
Sign in to reply to this message.
|