SOCIS2014 - Tcp Options Review

Code for Tcp Options

[n.p, 2014-07-07 08:18:38]

Here it is.. Thank you!

[Tom Henderson, 2014-07-14 19:28:36]

This is just an initial set of review comments/questions, limited mainly to the
proposed changes to test.h and the RttEstimator class and tests.

https://codereview.appspot.com/110860043/diff/1/src/core/model/test.h
File src/core/model/test.h (right):

https://codereview.appspot.com/110860043/diff/1/src/core/model/test.h#newcode739
src/core/model/test.h:739: * and abort if not.
I'm OK with these extensions, but wonder whether GT_OR_EQ variants should also
be provided for symmetry.

https://codereview.appspot.com/110860043/diff/1/src/internet/model/rtt-estima...
File src/internet/model/rtt-estimator.h (right):

https://codereview.appspot.com/110860043/diff/1/src/internet/model/rtt-estima...
src/internet/model/rtt-estimator.h:116: virtual uint32_t NowToTsValue ();
this method should probably be declared static since it does not use or change
state of the object.

If you do not want to state the units (so that the implementation could change
in the future), please state in the doxygen that clients should not assume that
the value is any particular unit (or else state that it is milliseconds).

https://codereview.appspot.com/110860043/diff/1/src/internet/model/rtt-estima...
src/internet/model/rtt-estimator.h:125: virtual Time EstimateRttFromTs (uint32_t
echoTime);
This is not very clear.  The implementation calls Measurement(), but it does not
return the value of RetransmitTimeout() but something different.

Can you clarify why (when) EstimateRttFromTs() should be called instead of the
sequence Measurement() and RetransmitTimeout()?

https://codereview.appspot.com/110860043/diff/1/src/internet/test/rtt-test.cc
File src/internet/test/rtt-test.cc (right):

https://codereview.appspot.com/110860043/diff/1/src/internet/test/rtt-test.cc...
src/internet/test/rtt-test.cc:121: NS_LOG_UNCOND ("Wait: " << m_timeToWait << "
last=" << lastValue << " start=" << m_initValue);
we typically do not put logging statements in test code (esp. UNCOND logging)

https://codereview.appspot.com/110860043/diff/1/src/internet/test/rtt-test.cc...
src/internet/test/rtt-test.cc:150: AddTestCase (new RttTestCase (200.0, 25.0,
.7, 18.5, 0.65), TestCase::QUICK);
can you explain (document) what is being tested here?  What is the reason for
selecting these new values?

[trucanh524, 2014-08-07 10:35:15]

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-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:330: 
A think a 1-line comment here (for example, // Padding) would be helpful.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
src/internet/model/tcp-header.cc:416: 
I remember the maximum option length allowed for each TCP segment is 40.  Should
we have a check for it here before appending any additional option?

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:35: TcpOptionEnd ();
Documentation for each method is recommended. Same for other classes.

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

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-sack.cc:62: 
Why don't we serialize and deserialize all the fields, including all the SACK
blocks and get the actual option size?

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

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-sack.h:20: 
I think that an Ascii art for the option structure (format) would be useful.

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

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-snack.h:32: 
Is this the SNACK option developed in the context of SCPS-TP?  If so, I don't
think this actually implements the SNACK  option structure as described in the
paper, which contains a 16-bit hole 1 offset, a 16-bit hole 1 size, and an
optional bit vector field in addition to the kind and length fields.  Its
structure is different from SACK, so I don't think it can be implemented in the
exact same way as SACK.

[mattator, 2014-08-07 14:36:37]

Hi,

Don't really know how to use this review tool. A check for TCP space is
mandatory in form of another. Having a function GetMaxTCPOptionSpace() is an
helper for people who wouldn't remember the value.

Another comment in TcpHeader::Deserialize:
Ptr<TcpOption> op = TcpOption::CreateOption (kind);

optionLen -= op->Deserialize (i);
You assume the option was correctly created. What if it were unknown ?




On 2014/08/07 10:35:15, trucanh524 wrote:
> 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-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:330: 
> A think a 1-line comment here (for example, // Padding) would be helpful.
> 
>
https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
> src/internet/model/tcp-header.cc:416: 
> I remember the maximum option length allowed for each TCP segment is 40. 
Should
> we have a check for it here before appending any additional option?
> 
>
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:35: TcpOptionEnd ();
> Documentation for each method is recommended. Same for other classes.
> 
>
https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
> File src/internet/model/tcp-option-sack.cc (right):
> 
>
https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
> src/internet/model/tcp-option-sack.cc:62: 
> Why don't we serialize and deserialize all the fields, including all the SACK
> blocks and get the actual option size?
> 
>
https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
> File src/internet/model/tcp-option-sack.h (right):
> 
>
https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
> src/internet/model/tcp-option-sack.h:20: 
> I think that an Ascii art for the option structure (format) would be useful.
> 
>
https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
> File src/internet/model/tcp-option-snack.h (right):
> 
>
https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
> src/internet/model/tcp-option-snack.h:32: 
> Is this the SNACK option developed in the context of SCPS-TP?  If so, I don't
> think this actually implements the SNACK  option structure as described in the
> paper, which contains a 16-bit hole 1 offset, a 16-bit hole 1 size, and an
> optional bit vector field in addition to the kind and length fields.  Its
> structure is different from SACK, so I don't think it can be implemented in
the
> exact same way as SACK.

[mattator, 2014-08-14 12:37:03]

When I reboot on a MPTCP kernel I will try to send a capture of this example if
you want.

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;
I believe you may have several options of a same kind. For instance MPTCP may
send several MPTCP options in a  same packet since MPTCP relies on different
options identified by different subtypes but that all share the same TCP option
id : the one of MPTCP.
So there should be a way to retrieve all Options in this case like:
GetOptions(std::list<>& list);

[Tommaso Pecorella, 2014-08-17 10:38:13]

Overall I like some parts of this patch, and I dislike some others.

The parts I like: the TCP options. The parts I dislike: the options handling.

I'd split the patch in three parts.
1) the test.cc additions 8to be committed as soon as possible).
2) the TCP options *alone*. No changes to the actual TCP code.
3) one (or even more) patches to handle the options.

The rationale for the 3-step is that almost all the options need to be:
a) negotiated in the 3-way handshake (I can't find it in this patch)
b) enabled / disabled according to the handshake
c) some have to be supported by TCP flavours. E.g., SACK, Window scaling, etc.

https://codereview.appspot.com/110860043/diff/20001/src/core/model/test.h
File src/core/model/test.h (left):

https://codereview.appspot.com/110860043/diff/20001/src/core/model/test.h#old...
src/core/model/test.h:737: * \ingroup testing
Please submit a patch with (only) the stuff in this file. Perhaps add it to
Bugzilla as an enhancement level thing.

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)
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.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
src/internet/model/tcp-header.cc:367: unsigned optionLen = (m_length - 5) * 4;
unsigned ?!?!?!

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);
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.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
src/internet/model/tcp-header.cc:386: }
I think we're going to far ahead with the buffer, we're advancing twice.

In any case, I would have done it in a different way.

while (optionLen)
{
 uint8_t kind = i.PeekU8 ();
 if (EndOfOptioList) optionLen = 0;
 else if (No-Operation) i.ReadU8 ();
 else
 {
    Ptr<TcpOption> op = TcpOption::CreateOption (kind);
    if (op)
    {
    optionLen -= op->Deserialize (i);
    m_options.push_back (op);
    i.Next (op->GetSerializedSize ()); // <- because we used peek
    }
    else
    {
NS_LOG_WARN ("Unknown option");
i.ReadU8 ();
uint8_t blobLen = i.ReadU8 ();
optionLen -= blobLen;
i.Next (blobLen-2);
    }
  }
}

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
src/internet/model/tcp-header.cc:416: 
Indeed, we must.

On 2014/08/07 10:35:14, trucanh524 wrote:
> I remember the maximum option length allowed for each TCP segment is 40. 
Should
> we have a check for it here before appending any additional option?

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;
I agree with the suggestion, but I'd rather pass a list in the return value.

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

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-end.cc:73: return 1;
This should be:

uint8_t readKind = i.ReadU8 ();
if (readKind != readKind)
  return 0;
return GetSerializedSize ();

As is:
1) always read the WHOLE thing, don't assume that part is deserialized
elsewhere.
2) if possible, check the consistency of the thing you read.

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
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.

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

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-sack-permitted.h:29: * Defines the TCP option of
kind 4 (Selective Acknowledgement Permitted) as in RFC2018
Merge SACK and SACK_PERMITTED (the headers and sources, not the classes!) - they
are from the same RFC.

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

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-sack.cc:60: return 2;
Bug: SACK is variable length.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-sack.cc:62: 
On 2014/08/07 10:35:14, trucanh524 wrote:
> Why don't we serialize and deserialize all the fields, including all the SACK
> blocks and get the actual option size? 

Because there's a bug... both Serialize and Deserialize are wrong (they don't
consider the actual data).

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-sack.cc:87: TcpOptionSack::AddSack (SackBlock s)
Issue an error if you try to add more than 4 blocks, as the overall option
length will exceed the max 40 bytes allowed.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-sack.cc:115: TcpOptionSack::ScoreBoard::iterator i
= m_sb.begin ();
1st: check that the requested offset is there.
2nd: don't make simple things complex.
    std::advance(i, offset);
see https://www.sgi.com/tech/stl/advance.html

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

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-sack.h:30: * Defines the TCP option of kind 5
(Selective Acknowledgement) as in RFC2018
\RFC{2018} (it adds automatically an hyperlink)

Add a \verbatim with the option structure (where it is good for understanding)

                         +--------+--------+
                         | Kind=5 | Length |
       +--------+--------+--------+--------+
       |      Left Edge of 1st Block       |
       +--------+--------+--------+--------+
       |      Right Edge of 1st Block      |
       +--------+--------+--------+--------+
       |                                   |
       /            . . .                  /
       |                                   |
       +--------+--------+--------+--------+
       |      Left Edge of nth Block       |
       +--------+--------+--------+--------+
       |      Right Edge of nth Block      |
       +--------+--------+--------+--------+

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-sack.h:39: typedef std::pair<SequenceNumber32,
SequenceNumber32> SackBlock;
Why use SequenceNumber and not "simply" uint32_t ?

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

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-snack.h:30: * Defines the TCP option of kind 21
(Selective Negative Acknowledgement)
Cite where the option is defined (there is no "official" RFC)

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

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-ts.h:30: * Defines the TCP option of kind 8
(timestamp option) as in RFC1323
TS and WINSCALE can be collapsed into one file, as they belong to the same rfc

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-ts.h:79: static Time EstimateRttFromTs (uint32_t
echoTime);
This function can be safely moved to the RTT estimator using this option.
I'd rather do this, because the real estimation is also dependent on a number of
other conditions, like duplicate ACK, ACK bit present, etc.

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: };
"OptionKind" and "Kind" seems to be the same stuff... kill one ?

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:51: #include "tcp-option-ts.h"
I'd rather use all the options, and add attributes for all of them. Unless there
is a good reason to not to.

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-so...
src/internet/model/tcp-socket-base.cc:2534: * to the header
The logic of this function should be triple checked.
If I do remember right, options should be negotiated during the 3-way handshake.

[n.p, 2014-08-23 10:30:37]

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:371: Ptr<TcpOption> op =
TcpOption::CreateOption (kind);
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.

Thank you

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
src/internet/model/tcp-header.cc:386: }
On 2014/08/17 10:38:12, Tommaso Pecorella wrote:
> I think we're going to far ahead with the buffer, we're advancing twice.

Where? 

> In any case, I would have done it in a different way.
> 
> while (optionLen)
> {
>  uint8_t kind = i.PeekU8 ();
>  if (EndOfOptioList) optionLen = 0;
>  else if (No-Operation) i.ReadU8 ();
>  else
>  {
>     Ptr<TcpOption> op = TcpOption::CreateOption (kind);
>     if (op)
>     {
>     optionLen -= op->Deserialize (i);
>     m_options.push_back (op);
>     i.Next (op->GetSerializedSize ()); // <- because we used peek
>     }
>     else
>     {
> NS_LOG_WARN ("Unknown option");
> i.ReadU8 ();
> uint8_t blobLen = i.ReadU8 ();
> optionLen -= blobLen;
> i.Next (blobLen-2);
>     }
>   }
> }
>  

Using peek is possible, but in every TcpOption* Deserialize method, an initial
read (to see what option is) is assumed.. I dont' know what is the better
approach here: use peek everywhere, modifying each Deserialize method, or not?

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/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.

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/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..

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-end.h:35: TcpOptionEnd ();
On 2014/08/07 10:35:14, trucanh524 wrote:
> Documentation for each method is recommended. Same for other classes. 

Only the virtual method is documented, not each instantiation, as what methods
do is pretty clear

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

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-snack.h:32: 
On 2014/08/07 10:35:14, trucanh524 wrote:
> Is this the SNACK option developed in the context of SCPS-TP?  If so, I don't
> think this actually implements the SNACK  option structure as described in the
> paper, which contains a 16-bit hole 1 offset, a 16-bit hole 1 size, and an
> optional bit vector field in addition to the kind and length fields.  Its
> structure is different from SACK, so I don't think it can be implemented in
the
> exact same way as SACK. 

Class removed as it isn't ready for publishing.

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

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-ts.h:79: static Time EstimateRttFromTs (uint32_t
echoTime);
On 2014/08/17 10:38:13, Tommaso Pecorella wrote:
> This function can be safely moved to the RTT estimator using this option.
> I'd rather do this, because the real estimation is also dependent on a number
of
> other conditions, like duplicate ACK, ACK bit present, etc.

As the Time to uint32_t conversion is dependent on what NowToTsValue does, I
think is better to maintain the uint32_t to Time conversion in the same file.
Ok, maybe the name could be changed.. because maybe can be another value (and
not an RTT).

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:51: #include "tcp-option-ts.h"
On 2014/08/17 10:38:13, Tommaso Pecorella wrote:
> I'd rather use all the options, and add attributes for all of them. Unless
there
> is a good reason to not to.

Use.. in what sense?

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/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.

[Tommaso Pecorella, 2014-08-25 14:40:15]

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:371: Ptr<TcpOption> op =
TcpOption::CreateOption (kind);
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 ... :)

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-he...
src/internet/model/tcp-header.cc:386: }
On 2014/08/23 10:30:36, n.p wrote:
> On 2014/08/17 10:38:12, Tommaso Pecorella wrote:
> > I think we're going to far ahead with the buffer, we're advancing twice.
> 
> Where? 

Maybe not, but I feel the code counterintuitive.


> > In any case, I would have done it in a different way.
> > 
> > while (optionLen)
> > {
> >  uint8_t kind = i.PeekU8 ();
> >  if (EndOfOptioList) optionLen = 0;
> >  else if (No-Operation) i.ReadU8 ();
> >  else
> >  {
> >     Ptr<TcpOption> op = TcpOption::CreateOption (kind);
> >     if (op)
> >     {
> >     optionLen -= op->Deserialize (i);
> >     m_options.push_back (op);
> >     i.Next (op->GetSerializedSize ()); // <- because we used peek
> >     }
> >     else
> >     {
> > NS_LOG_WARN ("Unknown option");
> > i.ReadU8 ();
> > uint8_t blobLen = i.ReadU8 ();
> > optionLen -= blobLen;
> > i.Next (blobLen-2);
> >     }
> >   }
> > }
> >  
> 
> Using peek is possible, but in every TcpOption* Deserialize method, an initial
> read (to see what option is) is assumed.. I dont' know what is the better
> approach here: use peek everywhere, modifying each Deserialize method, or not?

Peek everywhere, of course.
The point is: Serializing and Deserializing should read/write the same number of
bytes. Otherwise Deserialize would rely on the behaviour of external code (not
wise).
A Peek / Read redundancy is ok, those operations are not dramatically slow.

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

https://codereview.appspot.com/110860043/diff/20001/src/internet/model/tcp-op...
src/internet/model/tcp-option-ts.h:79: static Time EstimateRttFromTs (uint32_t
echoTime);
On 2014/08/23 10:30:37, n.p wrote:
> On 2014/08/17 10:38:13, Tommaso Pecorella wrote:
> > This function can be safely moved to the RTT estimator using this option.
> > I'd rather do this, because the real estimation is also dependent on a
number
> of
> > other conditions, like duplicate ACK, ACK bit present, etc.
> 
> As the Time to uint32_t conversion is dependent on what NowToTsValue does, I
> think is better to maintain the uint32_t to Time conversion in the same file.
> Ok, maybe the name could be changed.. because maybe can be another value (and
> not an RTT).

I guess it's a matter of taste. I like to have in the headers just the functions
related to the header read and write, not the ones processing the data carried
by the headers.
Still, I agree that the RTT estimation calc is in the same RFC, so the function
may "belong" to the same code piece.

However, leaving it here is "dangerous". The function uses a Simulator::Now()
internally, so two calls (by a dumb user) could generate two different results.
I'd rather have a EstimateRttFromTs (Time now, uint32_t echoTime). By the way,
this would also remove the need to make it static. It could be a member function
EstimateTimeDifference (Time now).

All, basically, are equivalent. They only differ in the eye of the coder.

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:51: #include "tcp-option-ts.h"
On 2014/08/23 10:30:37, n.p wrote:
> On 2014/08/17 10:38:13, Tommaso Pecorella wrote:
> > I'd rather use all the options, and add attributes for all of them. Unless
> there
> > is a good reason to not to.
> 
> Use.. in what sense?

tcp-option-mss.h, nop, etc... missing headers include. Why ?

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/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...

[Tom Henderson, 2014-08-26 16:52:30]

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