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

Issue 4875052: + Delaybox

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 10 months ago by Mat Kelly
Modified:
10 years, 7 months ago
Reviewers:
Tom Henderson
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

Port of Delaybox from ns-2

Patch Set 1 #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats (+1658 lines, -0 lines) Patch
A src/delaybox/delaybox.h View 1 chunk +292 lines, -0 lines 3 comments Download
A src/delaybox/delaybox.cc View 1 chunk +393 lines, -0 lines 2 comments Download
A src/delaybox/delaybox-net-device.h View 1 chunk +168 lines, -0 lines 1 comment Download
A src/delaybox/delaybox-net-device.cc View 1 chunk +118 lines, -0 lines 3 comments Download
A src/delaybox/tcp-flow-classifier.h View 1 chunk +112 lines, -0 lines 2 comments Download
A src/delaybox/tcp-flow-classifier.cc View 1 chunk +139 lines, -0 lines 1 comment Download
A src/delaybox/wscript View 1 chunk +16 lines, -0 lines 0 comments Download
A src/test/delaybox-test-suite.cc View 1 chunk +420 lines, -0 lines 0 comments Download

Messages

Total messages: 2
Mat Kelly
10 years, 10 months ago (2011-08-16 16:22:59 UTC) #1
Tom Henderson
10 years, 7 months ago (2011-11-15 19:10:18 UTC) #2
Sorry for the lengthy delay in reviewing this. Besides responding to the
detailed comments, it would help to provide some simple examples of how this can
be used.  Also, a documentation file (.rst) for the model library is needed.

I had a broader question about whether this should be implemented as a "Box"
(bump in the wire) or as packet treatment on a specialized NetDevice.  Couldn't
you accomplish everything you need by just making this a special NetDevice and
avoid e.g. the point-to-point variant?

Also, it seems like it would be trivial to make this also handle UDP or any
layer-4 protocol that matches the standard flowspec-- any reason why it is
limited to TCP?

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox-net-device.cc
File src/delaybox/delaybox-net-device.cc (right):

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox-net-device...
src/delaybox/delaybox-net-device.cc:64:
&DelayBoxNetDeviceT<ND>::m_delayBoxDropTrace)) //
an example demonstrating use of these trace sources would be helpful

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox-net-device...
src/delaybox/delaybox-net-device.cc:89: dest).Bind (protocolNumber);
This is an unconventional usage; I'm surprised that it works.  Why not use a
callback with explicit parameters?

Callback<bool, Ptr<Packet>, const Address&, uint16_t> ?

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox-net-device...
src/delaybox/delaybox-net-device.cc:90: if (m_delayBox->Delay (send,
static_cast<Ptr<const Packet> > (packet)))
is this static_cast needed?

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox-net-device.h
File src/delaybox/delaybox-net-device.h (right):

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox-net-device...
src/delaybox/delaybox-net-device.h:60: * DelayBox is a NetDevice that should be
placed on a node in between the source and destination nodes.
Can't this instead be used without an intermediate node?   The test suite seems
to use it that way.

Could you then do away with the DelayBoxNetDeviceT type of template class?  I am
not sure what the DelayBoxPointToPointNetDevice, for instance, buys you.

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox.cc
File src/delaybox/delaybox.cc (right):

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox.cc#newcode140
src/delaybox/delaybox.cc:140: // TODO: instead of digging through headers here,
we should take them as parameters to this function.
Doesn't seem robust to handling non-TCP (Ipv4) packets; have you tested it with
e.g. an IPv6 packet?

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox.cc#newcode253
src/delaybox/delaybox.cc:253: != m_rules.end () // match both ports
it might help for debugging to break each of these cases out into separate
tests, so you could put some debug log statements that express the specific
condition that matched.

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox.h
File src/delaybox/delaybox.h (right):

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox.h#newcode55
src/delaybox/delaybox.h:55: * \brief For internal use.
If you want this to be internal (not visible at the public API), suggest to
either 1) make a private member of DelayBoxRule, or 2) put in a separate header
file that is not exported by the wscript

Same comment applies for other \internal classes below

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox.h#newcode101
src/delaybox/delaybox.h:101: return m_delayRV;
we do not put implementation of methods into headers, unless necessary for
inline or templating.  Please put into the .cc file.

http://codereview.appspot.com/4875052/diff/1/src/delaybox/delaybox.h#newcode152
src/delaybox/delaybox.h:152: * \param send A callback which sends the packet
when called.
Incomplete doxygen for parameters.   Putting a TcpHeader here seems very
specialized; what if it is not TCP?

http://codereview.appspot.com/4875052/diff/1/src/delaybox/tcp-flow-classifier.cc
File src/delaybox/tcp-flow-classifier.cc (right):

http://codereview.appspot.com/4875052/diff/1/src/delaybox/tcp-flow-classifier...
src/delaybox/tcp-flow-classifier.cc:44: const uint8_t TCP_PROT_NUMBER = 6;
this constant also available as TcpL4Protocol::PROT_NUMBER

http://codereview.appspot.com/4875052/diff/1/src/delaybox/tcp-flow-classifier.h
File src/delaybox/tcp-flow-classifier.h (right):

http://codereview.appspot.com/4875052/diff/1/src/delaybox/tcp-flow-classifier...
src/delaybox/tcp-flow-classifier.h:69: * \param out_flowId If not null, the
packet's flow id will be written here on success.
A better C++ design would be to pass by non-const reference instead of by
pointer; then you don't have to worry about it being null.  Plus, we frown on
having raw pointers in our public API.

http://codereview.appspot.com/4875052/diff/1/src/delaybox/tcp-flow-classifier...
src/delaybox/tcp-flow-classifier.h:101: */
remove until implemented?
Sign in to reply to this message.

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