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

Issue 110860043: SOCIS2014 - Tcp Options Review (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 4 months ago by n.p
Modified:
6 years ago
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

Code for Tcp Options

Patch Set 1 #

Total comments: 5

Patch Set 2 : Worked on Tom's comments, patch set updated #

Total comments: 63

Patch Set 3 : SOCIS Final Review: Tcp Options RFC 793 #

Patch Set 4 : SOCIS Final Review: Tcp Option WinScale #

Patch Set 5 : SOCIS Final Review: Tcp Option TimeStamp #

Unified diffs Side-by-side diffs Delta from patch set Stats (+657 lines, -148 lines) Patch
M src/aodv/test/aodv-regression.cc View 1 2 3 4 2 chunks +2 lines, -1 line 0 comments Download
M src/aodv/test/bug-772.cc View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M src/aodv/test/loopback.cc View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M src/internet/model/rtt-estimator.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M src/internet/model/rtt-estimator.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M src/internet/model/tcp-option.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M src/internet/model/tcp-option.cc View 1 2 3 4 3 chunks +3 lines, -0 lines 0 comments Download
A src/internet/model/tcp-option-ts.h View 1 2 3 4 1 chunk +88 lines, -0 lines 0 comments Download
A src/internet/model/tcp-option-ts.cc View 1 2 3 4 1 chunk +152 lines, -0 lines 0 comments Download
M src/internet/model/tcp-socket-base.h View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M src/internet/model/tcp-socket-base.cc View 1 2 3 4 7 chunks +84 lines, -7 lines 0 comments Download
A src/internet/test/tcp-option-test.cc View 1 2 3 4 1 chunk +189 lines, -0 lines 0 comments Download
A + src/internet/test/tcp-timestamp-test.cc View 1 2 3 4 15 chunks +120 lines, -136 lines 0 comments Download
M src/internet/wscript View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M src/test/ns3tcp/ns3tcp-loss-test-suite.cc View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M src/test/ns3tcp/ns3tcp-state-test-suite.cc View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 9
n.p
Here it is.. Thank you!
7 years, 4 months ago (2014-07-07 08:18:38 UTC) #1
Tom Henderson
This is just an initial set of review comments/questions, limited mainly to the proposed changes ...
7 years, 4 months ago (2014-07-14 19:28:36 UTC) #2
trucanh524
Hi Natale, Follow are a few comments/questions I have on your code. Best, Anh https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-header.cc ...
7 years, 3 months ago (2014-08-07 10:35:15 UTC) #3
mattator
Hi, Don't really know how to use this review tool. A check for TCP space ...
7 years, 3 months ago (2014-08-07 14:36:37 UTC) #4
mattator
When I reboot on a MPTCP kernel I will try to send a capture of ...
7 years, 3 months ago (2014-08-14 12:37:03 UTC) #5
Tommaso Pecorella
Overall I like some parts of this patch, and I dislike some others. The parts ...
7 years, 3 months ago (2014-08-17 10:38:13 UTC) #6
n.p
https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-header.cc File src/internet/model/tcp-header.cc (right): https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-header.cc#newcode371 src/internet/model/tcp-header.cc:371: Ptr<TcpOption> op = TcpOption::CreateOption (kind); On 2014/08/17 10:38:12, Tommaso ...
7 years, 3 months ago (2014-08-23 10:30:37 UTC) #7
Tommaso Pecorella
https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-header.cc File src/internet/model/tcp-header.cc (right): https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-header.cc#newcode371 src/internet/model/tcp-header.cc:371: Ptr<TcpOption> op = TcpOption::CreateOption (kind); On 2014/08/23 10:30:36, n.p ...
7 years, 3 months ago (2014-08-25 14:40:15 UTC) #8
Tom Henderson
7 years, 3 months ago (2014-08-26 16:52:30 UTC) #9
some additional comments, focusing on the base TCP options (kinds 0,1,2) and
window scale, ignoring for now timestamps.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
File src/internet/model/tcp-header.cc (right):

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
src/internet/model/tcp-header.cc:80: TcpHeader::SetLength (uint8_t length)
On 2014/08/17 10:38:12, Tommaso Pecorella wrote:
> 1) Declare it obsolete.
> 2) Make it a no-op
> 3) Throw a warning if used.
> 
> Rationale: the TCP header length should be fixed, and dependent only on the
TCP
> header properties.
> 
> Note: this function is used (it's a bug). In the two cases where it is used,
> remove the call.

I agree to remove this.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
src/internet/model/tcp-header.cc:282: if ((m_flags & CWR) != 0)
whitespace changes should be outside of the options patch

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
src/internet/model/tcp-header.cc:330: 
On 2014/08/07 10:35:14, trucanh524 wrote:
> A think a 1-line comment here (for example, // Padding) would be helpful. 

Some implementations pad options to word boundaries, some do not.  It may be
useful to put a boolean attribute such as PadOptions (default false, for Linux
behavior) and make this padding depend on it.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
src/internet/model/tcp-header.cc:371: Ptr<TcpOption> op =
TcpOption::CreateOption (kind);
On 2014/08/25 14:40:15, Tommaso Pecorella wrote:
> On 2014/08/23 10:30:36, n.p wrote:
> > On 2014/08/17 10:38:12, Tommaso Pecorella wrote:
> > > This could return null, a check is mandatory.
> > > 
> > > Alternatively (and better) we should have a "Unknown" option type.
> > > I am not totally sure about the intended behaviour in case of unknown
> options
> > > (trash the packet or ignore the option ?), but we should be able to keep
on
> > with
> > > reading the packet.
> > 
> > All things done in Deserialize are assumed to be "ok".. but malformed packet
> can
> > crash the simulator. So, what about a isValid API? If packet has something
> > malformed, just stop and make the entire header invalid, thrashing the
packet.
> > 
> > Anyway, for now I'll insert an assert, just waiting more comments.
> 
> Thanks to the TCP Options structure (i.e., 2 options are 1-byte, the others
are
> Type-Length), an Unknown TCP option is possible. I'd go for that one, but the
> code just a few lines below works without an Unknown optino type ... :)

If kind is unknown, need to peek the length and deserialize the correct number
of bytes (handling the case of an unknown option preceding a known option) and
move to the next option.  I would also log this occurrence using NS_LOG
statement.  Check also that length of unknown option does not exceed the
remaining space of the options field; if it does, put in an assert until if/when
someone wants to suggest different behavior.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
File src/internet/model/tcp-header.h (right):

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
src/internet/model/tcp-header.h:162: Ptr<TcpOption> GetOption (uint8_t kind)
const;
On 2014/08/23 10:30:36, n.p wrote:
> On 2014/08/17 10:38:12, Tommaso Pecorella wrote:
> > I agree with the suggestion, but I'd rather pass a list in the return value.
> 
> Actually I don't know any of MPTCP (as the code isn't public). Anyway, this
call
> will remain: in some cases, the overhead of using lists in API isn't matched
by
> any improvement (for example, in the actual tcpSocketBase code). 
> 
> So, I suggest to include in the MPTCP patch a new method for this class, which
> works with list, as it will be the right place.

MPTCP has one instance of a TCP option 30, but within this option there are
subtypes.  

http://tools.ietf.org/html/rfc6824#section-8

I think that we are OK with the proposed GetOption () that returns a single
instance.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
src/internet/model/tcp-header.h:175: void AppendOption (Ptr<TcpOption> option);
what happens if this fails?  Is there a way to check if enough space exists to
add the option before calling this method?

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
File src/internet/model/tcp-option-end.h (right):

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-end.h:32: class TcpOptionEnd : public TcpOption
On 2014/08/23 10:30:36, n.p wrote:
> On 2014/08/17 10:38:13, Tommaso Pecorella wrote:
> > I'd avoid the file explosion. No-op, EoL and MSS can safely be in the "main"
> > TcpOption file.
> > 
> > Rationale: I would rather split the options according to their "belonging".
> > Original RFC, MP-TCP, etc.
> > 
> > Still, it's a matter of personal tastes.
> 
> IMHO, the result is the same, but we will end in:
> 
> #include "tcp-options-rfc2018"
> 
> vs.
> 
> #include "tcp-options-sack"
> 
> I think the 2nd is more KISS than the first. About the file explosion, is
> because the entire model is painful.. ip things should go in the ip
> subdirectory, tcp in their, and so on..

I would lean towards providing multiple classes (NOOP, End, MSS, SACK,
Timestamps) in a single option file.  These are typically used together, so they
can be included together.

if we provide the single option file, it could be instead named "tcp-options.h"
instead of "tcp-option.h"

Perhaps MPTCP is split out from the rest if it is quite lengthy.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
File src/internet/model/tcp-option-mss.h (right):

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-mss.h:30: */
It doesn't seem to me that this option is actually plumbed into the TCP logic.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
File src/internet/model/tcp-option-winscale.h (right):

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-winscale.h:62: * \brief Deconstructor
Destructor

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-winscale.h:99: virtual uint8_t GetKind (void)
const;
why virtual?

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
File src/internet/model/tcp-option.h (right):

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option.h:65: };
On 2014/08/17 10:38:13, Tommaso Pecorella wrote:
> "OptionKind" and "Kind" seems to be the same stuff... kill one ?

I agree to remove one of these enums.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-so...
File src/internet/model/tcp-socket-base.cc (right):

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:93: MakeBooleanChecker ())
is this supposed to be SACK?

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:2498: */
general comment:  Doxygen should be moved from implementation file to header
file

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:2518: ProcessOptionWScale
(header.GetOption (TcpOption::WINSCALE));
I might rewrite this as:

  if (m_winScalingEnabled && header.HasOption (TcpOption::WINSCALE))
    {
      NS_LOG_LOGIC ("Peer supports window scaling... keeping it enabled");
      ProcessOptionWScale (header.GetOption (TcpOption::WINSCALE));
    }
  else
    {
      NS_LOG_LOGIC ("Peer doesn't offer window scaling... disabling");
      m_winScalingEnabled = false;
    }

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:2534: * to the header
On 2014/08/25 14:40:15, Tommaso Pecorella wrote:
> On 2014/08/23 10:30:37, n.p wrote:
> > On 2014/08/17 10:38:13, Tommaso Pecorella wrote:
> > > The logic of this function should be triple checked.
> > > If I do remember right, options should be negotiated during the 3-way
> > handshake.
> > 
> > Some option can be inserted only in SYN packet (like WinScaling). Other
should
> > be enabled/disabled in 3-way-handshake, but if enabled, inserted in all
> packets.
> 
> That's my point. As I wrote in the (separate) mail, I think that we should
> triple check how the TCP connection is started. Offer the options we handle,
> check the reply, etc. At the end, we should have the list of supported option
> for the current connection (and use them).
> 
> Anyway, maybe I'm missing something (or I'm a bit tired). However I'm pretty
> sure that (for example) WindowScale is also "replied" in the SYN-ACK :P
> 
> As a side note... do we use the SACK option ? I fear not...

Does there need to be any provision for extensibility?  If a new TCP variant
(subclass) adds its own option, does tcp-socket-base need to be edited?

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:2600: m_rcvScaleFactor = 14;
isn't this a symptom of a protocol error?  I would possibly assert in this case.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:2603: m_sndScaleFactor = CalculateWScale
();
why is this being performed on the receive-side processing?  Can it be safely
deleted?

https://codereview.appspot.com/110860043/diff/20001/src/internet/test/tcp-opt...
File src/internet/test/tcp-option-test.cc (right):

https://codereview.appspot.com/110860043/diff/20001/src/internet/test/tcp-opt...
src/internet/test/tcp-option-test.cc:163: static class TcpOptionTestSuite :
public TestSuite
in general, missing from this test suite is handling of unknown options,
possibly different sizes, possibly in the "reserved" range of kind values.

https://codereview.appspot.com/110860043/diff/20001/src/internet/test/tcp-opt...
src/internet/test/tcp-option-test.cc:180: "values for timestamp",
what if we just test a few boundary conditions and a fixed pseudo-random (e.g.
0xdeadbeef) without randomization-- what does the random variable offer us?

https://codereview.appspot.com/110860043/diff/20001/src/internet/test/tcp-wsc...
File src/internet/test/tcp-wscaling-test.cc (right):

https://codereview.appspot.com/110860043/diff/20001/src/internet/test/tcp-wsc...
src/internet/test/tcp-wscaling-test.cc:23: #define protected public
making the test a friend class is possibly a better option than the above
Sign in to reply to this message.

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