Code review - Issue 5452045: TCP options feature for ns-3https://codereview.appspot.com/2014-04-18T17:09:29+00:00rietveld
Message from unknown
2011-12-03T22:44:29+00:00Adrianurn:md5:3ee6374f00de61123794ca3afa57ff29
Message from pdbj@mac.com
2012-01-04T18:55:18+00:00Peter Barnesurn:md5:c356bf52fef0a34cfdb0231fd66fe364
Minor suggestions:
http://codereview.appspot.com/5452045/diff/1/src/internet/model/tcp-option-sack.cc
File src/internet/model/tcp-option-sack.cc (right):
http://codereview.appspot.com/5452045/diff/1/src/internet/model/tcp-option-sack.cc#newcode52
src/internet/model/tcp-option-sack.cc:52: {
Sack printer would be really useful for illustration, debugging.
http://codereview.appspot.com/5452045/diff/1/src/internet/model/tcp-option-winscale.cc
File src/internet/model/tcp-option-winscale.cc (right):
http://codereview.appspot.com/5452045/diff/1/src/internet/model/tcp-option-winscale.cc#newcode60
src/internet/model/tcp-option-winscale.cc:60: return 3;
Should this be an enum or const in the class? It's used three places in this file, and similarly in the other options classes. Too many places to fix if the size ever changes, IMO.
http://codereview.appspot.com/5452045/diff/1/src/internet/model/tcp-option-winscale.cc#newcode67
src/internet/model/tcp-option-winscale.cc:67: i.WriteU8 (2); // Kind
Should this 'kind' be 3? See comment in tcp-option.cc
http://codereview.appspot.com/5452045/diff/1/src/internet/model/tcp-option-winscale.cc#newcode68
src/internet/model/tcp-option-winscale.cc:68: i.WriteU8 (3); // Length
Is this the 'size' var in Deserialize? Can we use the same label for the var and the comment?
http://codereview.appspot.com/5452045/diff/1/src/internet/model/tcp-option.cc
File src/internet/model/tcp-option.cc (right):
http://codereview.appspot.com/5452045/diff/1/src/internet/model/tcp-option.cc#newcode60
src/internet/model/tcp-option.cc:60: if (kind == 0)
Should these 'kind' constants be enums in this base class? This would avoid problems like tcp-option-winscale.cc where kind is serialized as 2 but GetKind returns 3.
Message from tomh.org@gmail.com
2014-04-18T17:05:46+00:00Tom Hendersonurn:md5:46875212e507111d8d339090fdfe7d0a
These need some additional work, since (if I recall correctly the last time I tested this patch) they can crash when run against an implementation TCP that generates additional options like SackPermitted). Peter also raises some good suggestions.
I've suggested to Anh to try to address Peter's comments, and then I'd like to proceed with these steps:
1) get Adrian's base TCP option code in with support for option kinds 0, 1, 2, and > 2. The code should not crash when run against a TCP that generates options (i.e. a real TCP that generates Sack permitted, or possibly others that come in the SYN). A TCP test that tests the deserialization logic for specially constructed test TCP options would be useful. For instance, create two test options that, when included, require also the use of No-op option and padding.
Once we are OK with this design, then add Window Scale and Timestamp as separate patches on top of this.
Message from tomh.org@gmail.com
2014-04-18T17:09:29+00:00Tom Hendersonurn:md5:a9a7d80fd7c143c481b7f9d08763d837
oops, I posted the most recent comments against the wrong issue.
I meant to post against
https://codereview.appspot.com/24900043
it may be the case that once 24900043 is done, we can pull any additional options from here (e.g. SackPermitted).