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

Issue 1690056: NS-3 Click Integration Code Review (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 7 months ago by Lalith Suresh
Modified:
9 years, 11 months ago
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Patch Set 1 #

Total comments: 40

Patch Set 2 : Corrections after Josh' code review #

Total comments: 2

Patch Set 3 : Changes included after Manuel Requena's code review #

Patch Set 4 : Fixed Doxygen warnings and added manual entry #

Total comments: 8

Patch Set 5 : Corrections after Tom's review #

Patch Set 6 : Memory Leak fix #

Patch Set 7 : Full Patch Set #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4849 lines, -21 lines) Patch
A doc/manual/click.texi View 4 5 1 chunk +211 lines, -0 lines 0 comments Download
M doc/manual/manual.texi View 2 chunks +2 lines, -0 lines 0 comments Download
A examples/click/nsclick-ip-router.click View 1 3 1 chunk +82 lines, -0 lines 0 comments Download
A examples/click/nsclick-raw-wlan.cc View 1 3 1 chunk +141 lines, -0 lines 0 comments Download
A examples/click/nsclick-raw-wlan.click View 1 3 1 chunk +105 lines, -0 lines 0 comments Download
A examples/click/nsclick-routing.cc View 1 3 1 chunk +132 lines, -0 lines 0 comments Download
A examples/click/nsclick-routing-node0.click View 1 3 1 chunk +99 lines, -0 lines 0 comments Download
A examples/click/nsclick-routing-node2.click View 1 3 1 chunk +99 lines, -0 lines 0 comments Download
A examples/click/nsclick-simple-lan.cc View 1 2 3 1 chunk +99 lines, -0 lines 0 comments Download
A examples/click/nsclick-simple-lan.click View 1 3 1 chunk +100 lines, -0 lines 0 comments Download
A examples/click/nsclick-udp-client-server-csma.cc View 1 3 1 chunk +131 lines, -0 lines 0 comments Download
A examples/click/nsclick-udp-client-server-wifi.cc View 1 3 1 chunk +162 lines, -0 lines 0 comments Download
A examples/click/wscript View 1 chunk +22 lines, -0 lines 0 comments Download
M src/devices/csma/csma-net-device.cc View 1 2 3 4 chunks +11 lines, -3 lines 0 comments Download
M src/devices/wifi/mac-low.cc View 3 chunks +23 lines, -1 line 0 comments Download
A src/helper/click-internet-stack-helper.h View 1 2 5 1 chunk +242 lines, -0 lines 0 comments Download
A src/helper/click-internet-stack-helper.cc View 1 2 5 1 chunk +632 lines, -0 lines 0 comments Download
M src/helper/wscript View 3 chunks +3 lines, -1 line 0 comments Download
A src/internet-stack/ipv4-l3-click-protocol.h View 1 4 5 1 chunk +263 lines, -0 lines 0 comments Download
A src/internet-stack/ipv4-l3-click-protocol.cc View 1 5 1 chunk +781 lines, -0 lines 0 comments Download
M src/internet-stack/tcp-l4-protocol.cc View 1 2 6 chunks +49 lines, -8 lines 0 comments Download
M src/internet-stack/udp-l4-protocol.cc View 1 2 4 chunks +46 lines, -6 lines 0 comments Download
M src/internet-stack/udp-socket-impl.cc View 1 2 chunks +11 lines, -1 line 0 comments Download
M src/internet-stack/wscript View 2 chunks +2 lines, -0 lines 0 comments Download
A src/routing/click/click.h View 1 3 1 chunk +106 lines, -0 lines 0 comments Download
A src/routing/click/ipv4-click-routing.h View 1 chunk +216 lines, -0 lines 0 comments Download
A src/routing/click/ipv4-click-routing.cc View 1 5 6 1 chunk +585 lines, -0 lines 0 comments Download
A src/routing/click/ipv4-click-routing-test.cc View 1 2 5 1 chunk +225 lines, -0 lines 0 comments Download
A src/routing/click/ipv4-external-routing.h View 1 2 4 5 1 chunk +96 lines, -0 lines 0 comments Download
A src/routing/click/ipv4-external-routing.cc View 1 5 1 chunk +77 lines, -0 lines 0 comments Download
A src/routing/click/waf View 1 chunk +1 line, -0 lines 0 comments Download
A src/routing/click/wscript View 1 5 1 chunk +88 lines, -0 lines 0 comments Download
M src/wscript View 3 chunks +2 lines, -1 line 0 comments Download
M test.py View 2 chunks +4 lines, -0 lines 0 comments Download
M wscript View 1 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 7
Josh Pelkey
Lalith, This is my initial review. Aside from the examples/test not working for me (although ...
10 years, 7 months ago (2010-08-02 21:51:31 UTC) #1
Lalith Suresh
On 2010/08/02 21:51:31, Josh Pelkey wrote: > Lalith, > > This is my initial review. ...
10 years, 7 months ago (2010-08-03 05:39:30 UTC) #2
Lalith Suresh
http://codereview.appspot.com/1690056/diff/1/7 File examples/click/nsclick-routing.cc (right): http://codereview.appspot.com/1690056/diff/1/7#newcode25 examples/click/nsclick-routing.cc:25: // - All nodes are Click based. On 2010/08/02 ...
10 years, 7 months ago (2010-08-03 07:17:11 UTC) #3
Manuel Requena
Lalith, Great job! Your work will be very useful for ns-3 and click users. I ...
10 years, 7 months ago (2010-08-05 10:38:10 UTC) #4
Lalith Suresh
Hello Manuel, Thanks for the review! I've incorporated the following changes as you'd suggested: 1) ...
10 years, 7 months ago (2010-08-05 15:01:15 UTC) #5
Tom Henderson
Overall this looks very nice but I had some suggestions for a few issues (documentation, ...
10 years, 7 months ago (2010-08-11 23:56:39 UTC) #6
Lalith Suresh
10 years, 7 months ago (2010-08-12 07:05:54 UTC) #7
Hello Tom,

Thanks for the very detailed review! I've addressed most of the issues that
you'd raised, the comments can be found inline. The latest patch set has been
uploaded as well.

http://codereview.appspot.com/1690056/diff/1/9
File examples/click/nsclick-simple-lan.click (right):

http://codereview.appspot.com/1690056/diff/1/9#newcode8
examples/click/nsclick-simple-lan.click:8: //  commercial product is hereby
granted without fee, provided that the
On 2010/08/11 23:56:39, Tom Henderson wrote:
> Unfortunately, this license is not GPL-compatible.  Can you ask the authors
> whether it can be replaced with a GPL-compatible license for inclusion in
ns-3?

I've mailed them regarding this, waiting for their response.

http://codereview.appspot.com/1690056/diff/1/14
File src/devices/wifi/mac-low.cc (right):

http://codereview.appspot.com/1690056/diff/1/14#newcode890
src/devices/wifi/mac-low.cc:890: NetDevice::PacketType(0));
On 2010/08/11 23:56:39, Tom Henderson wrote:
> We'll need to consult the wifi maintainers about a way to better support this.

This can be done without introducing a dependency here _as of now_. This would
involve letting the packets go up a NetDevice normally, and pass on a resulting
IP packet to Click, ensuring that the Click graph expects an IP packet only. But
the problem is, when the time comes to add Click MAC support, we'll need the
code to work in the manner that has been implemented right now (pass packets
directly to Click with the L2 headers on). But yes, I really could use some
feedback on how to handle this!

http://codereview.appspot.com/1690056/diff/1/15
File src/helper/click-internet-stack-helper.cc (right):

http://codereview.appspot.com/1690056/diff/1/15#newcode146
src/helper/click-internet-stack-helper.cc:146: * "Drop" (see
ns3::UdpSocketImpl::m_dropTrace).  This is set when a packet
On 2010/08/11 23:56:39, Tom Henderson wrote:
> Is this the right place for documentation of transport protocol tracing?

This is documentation adapted from internet-stack-helper.cc.

http://codereview.appspot.com/1690056/diff/1/16
File src/helper/click-internet-stack-helper.h (right):

http://codereview.appspot.com/1690056/diff/1/16#newcode144
src/helper/click-internet-stack-helper.h:144: void SetIpv4StackInstall (bool
enable);
On 2010/08/11 23:56:39, Tom Henderson wrote:
> Why is this needed?  In what cases would you set to false and then run
> Install()?

This isn't required here. I'll remove it.

http://codereview.appspot.com/1690056/diff/1/24
File src/routing/click/click.h (right):

http://codereview.appspot.com/1690056/diff/1/24#newcode68
src/routing/click/click.h:68: *   - Only userlevel elements can be used.
On 2010/08/11 23:56:39, Tom Henderson wrote:
> What does this mean (to the newbie that may not know click)?  Can you briefly
> list (here and the manual) what elements in click are not userlevel?

We basically have userlevel and kernel level elements in Click. The nsclick
simulator API works only at userlevel, thus enforcing this. I'll document this
properly now.

http://codereview.appspot.com/1690056/diff/1/26
File src/routing/click/ipv4-click-routing.cc (right):

http://codereview.appspot.com/1690056/diff/1/26#newcode286
src/routing/click/ipv4-click-routing.cc:286: // feed dummy values into pinfo.
This avoids the need to make changes in the Click code
On 2010/08/11 23:56:39, Tom Henderson wrote:
> What are the implications of feeding dummy data to these fields?  Will this
> break some click components?

No implications here. This was a requirement with ns-2 to facilitate a tracing
subsystem within Click for ns-2 packets. With ns-3, this requirement isn't
there, since we can either Print packet contents to stdout or generate pcap
traces of packet flows from any point inside Click as required. And yes, I need
to document this better too. :)

http://codereview.appspot.com/1690056/diff/1/31
File src/routing/click/wscript (right):

http://codereview.appspot.com/1690056/diff/1/31#newcode18
src/routing/click/wscript:18: nsclick_dir = os.path.join('..','click','ns')
On 2010/08/11 23:56:39, Tom Henderson wrote:
> This guess will likely fail for most users, since click will be at a directory
> above but be named something like "click-1.8.0".  Can you make this guess
1.8.0
> or do regexp matching?

The git clone of the Click repository gives a directory named 'click' by
default. I thought this would be a better guess, since Click doesn't release
frequently, and active users/developers are encouraged to use the git sources.

> 
> Do you want to enforce click version greater than a minimal version? 

I've tested only with click-1.8.0 (latest stable release) and the git clone. I'm
not sure if I should enforce a version. I'll ask Ruben and get back to you on
this.

http://codereview.appspot.com/1690056/diff/12001/13008
File src/routing/click/ipv4-external-routing.h (right):

http://codereview.appspot.com/1690056/diff/12001/13008#newcode34
src/routing/click/ipv4-external-routing.h:34: */
On 2010/08/11 23:56:39, Tom Henderson wrote:
> This does not sufficiently explain why this class is needed.  What do we lose
by
> deleting this class and making Ipv4ClickRouting derive directly from
> Ipv4RoutingProtocol?

As of now, we don't lose anything if we derive directly from
Ipv4RoutingProtocol. I just thought we could use this class to enforce Send(),
Receive() methods in case any more external routers are to be integrated in the
future. Note that we can add this class rather cleanly in the future as well, so
I can strip this out if you think it necessary?

http://codereview.appspot.com/1690056/diff/26001/27001
File doc/manual/click.texi (right):

http://codereview.appspot.com/1690056/diff/26001/27001#newcode134
doc/manual/click.texi:134: @item For any node, the 0th device will be tap0. The
remaining devices should be eth0, eth1 and so forth (even if you're using wifi).
Please note that the device numbering should begin from 0.
On 2010/08/11 23:56:40, Tom Henderson wrote:
> this plumbing description is a bit obscure-- can you elaborate more what is
> going on?  what is the kernel in this case?  why are tap devices being used? 
> where does this 0th device convention come from?
> 
> Elsewhere in the manual, there is a "path of a packet" diagram sketching how a
> packet traverses an InternetStack.  Please consider to do similarly a simple
use
> case and put it into the "Design" section of this chapter, if you ever get
time
> for that.  For instance, walk through what happens on the middle node on the
> nsclick-routing.cc example.
>   

The device naming convention for the Click files (including that of 'tap0' for
the kernel), was borrowed as is from the original nsclick implementation. As of
now, I've hard coded this into Ipv4ClickRouting. Since this point was raised by
another reviewer as well, I'm trying to think of a way to keep it flexible.
Probably maintain an "ns-3 <-> click" device map inside Ipv4ClickRouting, which
can be freely set by the user in the simulation script?

And I'll add the packet path diagram as well.

http://codereview.appspot.com/1690056/diff/26001/27001#newcode171
doc/manual/click.texi:171: @end enumerate
On 2010/08/11 23:56:40, Tom Henderson wrote:
> These are all very simple examples.  How would one construct, say, a 100-node
> topology?  Are there tools available to generate click configs from higher
layer
> scenario descriptions?  If so, can you point users to them?

Since we're using Click for routing, the examples were meant to just show
different use cases that we could support. Hybrid scenarios (simple-lan,
raw-wlan), Only-Click-node scenarios (udp-client-server examples) and a static
routing based Click router (nsclick-routing). Some configurations (like the IP
router being used in nsclick-routing) can be generated from scripts that are
packaged with Click. For others, the users have to write them themselves. I'll
specify the paths within the Click source tree where these scripts can be found.

For something like a 100-node topology, there is again no difference in the way
Click will be used. One just needs to install a ClickInternetStackHelper on the
nodes that are to run Click, specify the appropriate Click files to be used, and
they're good to go.

http://codereview.appspot.com/1690056/diff/26001/27001#newcode182
doc/manual/click.texi:182: @end enumerate
On 2010/08/11 23:56:40, Tom Henderson wrote:
> I had a look at the tests and the regression test is fairly rudimentary;
testing
> bindings between ns-3 and click names.  Can you please specify more detail
here
> about the test topology and what specifically you asked Click router to do? 
> 
> This gets back to the documentation issue about what is and what is not
> supported.  I would list here explicitly which elements were tested to work
> fine, and state that other elements are unknown (and more tests are welcome
from
> future users).
> 
> Can you copy some examples (where packets are flowing) to the test directory,
> for regression purposes?

Yes, I'll specify more details about what has been tested. Basically,
ipv4-click-routing-test.cc was meant to only test whether the methods inside
Ipv4ClickRouting class were working as expected. The example scripts are the
actual tests for the integration. The examples cover the following:

1) Using TCP/UDP on top of Click, which was not possible on the original nsclick
implementation.
2) Having Click based and non-click based nodes communicate with each other, in
both csma and wifi situations. (nsclick-simple-lan/raw-wlan)
3) Routing using Click. (nsclick-routing.cc)

I will elaborate on these specifics in the Validation section to make it more
clear.
Sign in to reply to this message.

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