Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1510)

Issue 4608045: Bug 1102 - IPv4 header fix (Closed)

Can't Edit
Can't Publish+Mail
Start Review
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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+18 lines, -11 lines) Patch
M src/internet/model/ipv4-header.h View 1 3 chunks +7 lines, -4 lines 0 comments Download
M src/internet/model/ipv4-header.cc View 1 2 3 4 4 chunks +11 lines, -7 lines 0 comments Download

Messages

Total messages: 30
Tommaso Pecorella
Consider this as a possible patch to fix bug # 1102
12 years, 9 months ago (2011-06-15 12:07:25 UTC) #1
Tom Henderson
I would also suggest that you change this in the Print() method: - << "offset ...
12 years, 9 months ago (2011-06-15 16:33:23 UTC) #2
Tommaso Pecorella
12 years, 9 months ago (2011-06-15 22:09:05 UTC) #3
Tommaso Pecorella
On 2011/06/15 22:09:05, Tommaso Pecorella wrote: Sorry if I push for it, but we need ...
12 years, 9 months ago (2011-06-20 19:48:49 UTC) #4
John Abraham
Some more comments http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-header.cc File src/internet/model/ipv4-header.cc (right): http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-header.cc#newcode120 src/internet/model/ipv4-header.cc:120: NS_ABORT_MSG_IF ((offsetBytes | 0x7), "offsetBytes must ...
12 years, 9 months ago (2011-06-20 20:53:20 UTC) #5
John Abraham
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-header.h#newcode116 src/internet/model/ipv4-header.h:116: * \returns the offset of this ...
12 years, 9 months ago (2011-06-20 21:01:09 UTC) #6
Tommaso Pecorella
http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-header.cc File src/internet/model/ipv4-header.cc (right): http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-header.cc#newcode120 src/internet/model/ipv4-header.cc:120: NS_ABORT_MSG_IF ((offsetBytes | 0x7), "offsetBytes must be multiple of ...
12 years, 9 months ago (2011-06-20 22:42:31 UTC) #7
John Abraham
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-header.h#newcode116 src/internet/model/ipv4-header.h:116: * \returns the offset of this fragment measured in ...
12 years, 9 months ago (2011-06-20 22:50:32 UTC) #8
John Abraham
http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-header.cc File src/internet/model/ipv4-header.cc (right): http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-header.cc#newcode120 src/internet/model/ipv4-header.cc:120: NS_ABORT_MSG_IF ((offsetBytes | 0x7), "offsetBytes must be multiple of ...
12 years, 9 months ago (2011-06-20 23:01:21 UTC) #9
John Abraham
On 2011/06/20 23:01:21, johnjoe7 wrote: > http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-header.cc > File src/internet/model/ipv4-header.cc (right): > > http://codereview.appspot.com/4608045/diff/6001/src/internet/model/ipv4-header.cc#newcode120 > ...
12 years, 9 months ago (2011-06-20 23:39:22 UTC) #10
John Abraham
12 years, 9 months ago (2011-06-20 23:39:43 UTC) #11
Tommaso Pecorella
12 years, 9 months ago (2011-06-21 11:32:29 UTC) #12
Tom Henderson
I'm fine with this version. I would also be fine with adding an additional check ...
12 years, 9 months ago (2011-06-21 13:36:00 UTC) #13
Tommaso Pecorella
On 2011/06/21 13:36:00, Tom Henderson wrote: > I'm fine with this version. I would also ...
12 years, 9 months ago (2011-06-21 14:20:18 UTC) #14
John Abraham
On 2011/06/21 14:20:18, Tommaso Pecorella wrote: > On 2011/06/21 13:36:00, Tom Henderson wrote: > > ...
12 years, 9 months ago (2011-06-21 14:54:18 UTC) #15
Tom Henderson
On 2011/06/21 14:20:18, Tommaso Pecorella wrote: > On 2011/06/21 13:36:00, Tom Henderson wrote: > > ...
12 years, 9 months ago (2011-06-21 15:39:00 UTC) #16
Tommaso Pecorella
On 2011/06/21 14:54:18, johnjoe7 wrote: > Well we wanted to make this API usable by ...
12 years, 9 months ago (2011-06-21 17:03:06 UTC) #17
Tommaso Pecorella
12 years, 9 months ago (2011-06-21 17:04:04 UTC) #18
Tom Henderson
On 2011/06/21 17:03:06, Tommaso Pecorella wrote: > This is an horribly wrong statement, as the ...
12 years, 9 months ago (2011-06-21 18:21:37 UTC) #19
John Abraham
On 2011/06/21 17:03:06, Tommaso Pecorella wrote: > On 2011/06/21 14:54:18, johnjoe7 wrote: > > Well ...
12 years, 9 months ago (2011-06-21 18:40:41 UTC) #20
John Abraham
12 years, 9 months ago (2011-06-21 18:40:48 UTC) #21
John Abraham
Total Length: 16 bits The total length of an IP datagram which includes IP hdr ...
12 years, 9 months ago (2011-06-21 19:24:56 UTC) #22
John Abraham
Also note: although this issue is irrelevant now due to consensus. I just wanted to ...
12 years, 9 months ago (2011-06-21 19:40:52 UTC) #23
Tommaso Pecorella
Very interesting topic indeed, and it might be relevant on the fragmentation / reassembly (different ...
12 years, 9 months ago (2011-06-21 21:08:38 UTC) #24
John Abraham
On 2011/06/21 21:08:38, Tommaso Pecorella wrote: > Very interesting topic indeed, and it might be ...
12 years, 9 months ago (2011-06-21 21:13:05 UTC) #25
Tom Henderson
http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-header.cc File src/internet/model/ipv4-header.cc (right): http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-header.cc#newcode128 src/internet/model/ipv4-header.cc:128: { I did not follow the rationale for adding ...
12 years, 9 months ago (2011-06-21 21:56:30 UTC) #26
Tommaso Pecorella
http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-header.cc File src/internet/model/ipv4-header.cc (right): http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-header.cc#newcode128 src/internet/model/ipv4-header.cc:128: { On 2011/06/21 21:56:30, Tom Henderson wrote: > I ...
12 years, 9 months ago (2011-06-22 00:03:36 UTC) #27
Tom Henderson
On 2011/06/22 00:03:36, Tommaso Pecorella wrote: > http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-header.cc > File src/internet/model/ipv4-header.cc (right): > > http://codereview.appspot.com/4608045/diff/21001/src/internet/model/ipv4-header.cc#newcode128 ...
12 years, 9 months ago (2011-06-22 05:08:26 UTC) #28
Tommaso Pecorella
12 years, 9 months ago (2011-06-22 22:09:49 UTC) #29
Tommaso Pecorella
12 years, 9 months ago (2011-06-22 22:11:01 UTC) #30
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b