NS-3 Click Integration Code Review

[Josh Pelkey, 2010-08-02 21:51:31]

Lalith,

This is my initial review.  Aside from the examples/test not working for me
(although I had them working a few weeks ago), I think all of this looks pretty
excellent.  Great job so far!  See inline comments below.

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.
I get this error when I run this example (something similar happens to the other
examples): 

examples/click/nsclick-routing-node0.click:37: While configuring
‘u/myarpresponder :: ARPResponder’:
  warning: multiple entries for 172.16.1.1/32
examples/click/nsclick-ip-router.click:31: While configuring ‘u/ar0 ::
ARPResponder’:
  warning: multiple entries for 172.16.1.2/32
examples/click/nsclick-ip-router.click:42: While configuring ‘u/ar1 ::
ARPResponder’:
  warning: multiple entries for 172.16.2.1/32
examples/click/nsclick-routing-node2.click:37: While configuring
‘u/myarpresponder :: ARPResponder’:
  warning: multiple entries for 172.16.2.2/32

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

http://codereview.appspot.com/1690056/diff/1/8#newcode59
examples/click/nsclick-simple-lan.cc:59: clickinternet.SetClickFile
(csmaNodes.Get (0),
"/home/nightstrike/builds/gsoc/testnew/ns-3-click/examples/click/nsclick-simple-lan.click");
whoops, don't want that full path there :)

http://codereview.appspot.com/1690056/diff/1/13
File src/devices/csma/csma-net-device.cc (left):

http://codereview.appspot.com/1690056/diff/1/13#oldcode912
src/devices/csma/csma-net-device.cc:912: // Only transmit if send side of net
device is enabled
not sure how this one got in here

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#newcode19
src/helper/click-internet-stack-helper.cc:19: * Author: Faker Moatamri
<faker.moatamri@sophia.inria.fr>
change to or add yourself?

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#newcode163
src/helper/click-internet-stack-helper.h:163: * \param clickfile Click file to
be used
wrong doxygen, rt instead of clickfile

http://codereview.appspot.com/1690056/diff/1/16#newcode170
src/helper/click-internet-stack-helper.h:170: * \param clickfile Click file to
be used
wrong doxygen, rt instead of clickfile

http://codereview.appspot.com/1690056/diff/1/20
File src/internet-stack/tcp-l4-protocol.cc (right):

http://codereview.appspot.com/1690056/diff/1/20#newcode386
src/internet-stack/tcp-l4-protocol.cc:386: isClickNode = true;
either put this on one line, or use braces

http://codereview.appspot.com/1690056/diff/1/21
File src/internet-stack/udp-l4-protocol.cc (right):

http://codereview.appspot.com/1690056/diff/1/21#newcode89
src/internet-stack/udp-l4-protocol.cc:89: isClickNode = true;
same as before

http://codereview.appspot.com/1690056/diff/1/22
File src/internet-stack/udp-socket-impl.cc (left):

http://codereview.appspot.com/1690056/diff/1/22#oldcode70
src/internet-stack/udp-socket-impl.cc:70: m_allowBroadcast = false;
is this stuff part of click?

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

http://codereview.appspot.com/1690056/diff/1/25#newcode50
src/routing/click/ipv4-click-routing-test.cc:50: click->SetClickFile
("nsclick-simple-lan.click");
will this path work, or do you need to specify examples/...

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

http://codereview.appspot.com/1690056/diff/1/29#newcode31
src/routing/click/ipv4-external-routing.h:31: class Ipv4ExternalRouting : public
Ipv4RoutingProtocol
maybe a description here about why you need this class

http://codereview.appspot.com/1690056/diff/1/32
File src/routing/olsr/olsr-routing-protocol.h (left):

http://codereview.appspot.com/1690056/diff/1/32#oldcode179
src/routing/olsr/olsr-routing-protocol.h:179: virtual bool RouteInput 
(Ptr<const Packet> p, const Ipv4Header &header, Ptr<const NetDevice> idev,
dont need this

[Lalith Suresh, 2010-08-03 05:39:30]

On 2010/08/02 21:51:31, Josh Pelkey wrote:
> Lalith,
> 
> This is my initial review.  Aside from the examples/test not working for me
> (although I had them working a few weeks ago), I think all of this looks
pretty
> excellent.  Great job so far!  See inline comments below.
> 
> 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.
> I get this error when I run this example (something similar happens to the
other
> examples): 
> 
> examples/click/nsclick-routing-node0.click:37: While configuring
> ‘u/myarpresponder :: ARPResponder’:
>   warning: multiple entries for 172.16.1.1/32
> examples/click/nsclick-ip-router.click:31: While configuring ‘u/ar0 ::
> ARPResponder’:
>   warning: multiple entries for 172.16.1.2/32
> examples/click/nsclick-ip-router.click:42: While configuring ‘u/ar1 ::
> ARPResponder’:
>   warning: multiple entries for 172.16.2.1/32
> examples/click/nsclick-routing-node2.click:37: While configuring
> ‘u/myarpresponder :: ARPResponder’:
>   warning: multiple entries for 172.16.2.2/32
> 
> http://codereview.appspot.com/1690056/diff/1/8
> File examples/click/nsclick-simple-lan.cc (right):
> 
> http://codereview.appspot.com/1690056/diff/1/8#newcode59
> examples/click/nsclick-simple-lan.cc:59: clickinternet.SetClickFile
> (csmaNodes.Get (0),
>
"/home/nightstrike/builds/gsoc/testnew/ns-3-click/examples/click/nsclick-simple-lan.click");
> whoops, don't want that full path there :)
> 
> http://codereview.appspot.com/1690056/diff/1/13
> File src/devices/csma/csma-net-device.cc (left):
> 
> http://codereview.appspot.com/1690056/diff/1/13#oldcode912
> src/devices/csma/csma-net-device.cc:912: // Only transmit if send side of net
> device is enabled
> not sure how this one got in here
> 
> 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#newcode19
> src/helper/click-internet-stack-helper.cc:19: * Author: Faker Moatamri
> <mailto:faker.moatamri@sophia.inria.fr>
> change to or add yourself?
> 
> 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#newcode163
> src/helper/click-internet-stack-helper.h:163: * \param clickfile Click file to
> be used
> wrong doxygen, rt instead of clickfile
> 
> http://codereview.appspot.com/1690056/diff/1/16#newcode170
> src/helper/click-internet-stack-helper.h:170: * \param clickfile Click file to
> be used
> wrong doxygen, rt instead of clickfile
> 
> http://codereview.appspot.com/1690056/diff/1/20
> File src/internet-stack/tcp-l4-protocol.cc (right):
> 
> http://codereview.appspot.com/1690056/diff/1/20#newcode386
> src/internet-stack/tcp-l4-protocol.cc:386: isClickNode = true;
> either put this on one line, or use braces
> 
> http://codereview.appspot.com/1690056/diff/1/21
> File src/internet-stack/udp-l4-protocol.cc (right):
> 
> http://codereview.appspot.com/1690056/diff/1/21#newcode89
> src/internet-stack/udp-l4-protocol.cc:89: isClickNode = true;
> same as before
> 
> http://codereview.appspot.com/1690056/diff/1/22
> File src/internet-stack/udp-socket-impl.cc (left):
> 
> http://codereview.appspot.com/1690056/diff/1/22#oldcode70
> src/internet-stack/udp-socket-impl.cc:70: m_allowBroadcast = false;
> is this stuff part of click?
> 
> http://codereview.appspot.com/1690056/diff/1/25
> File src/routing/click/ipv4-click-routing-test.cc (right):
> 
> http://codereview.appspot.com/1690056/diff/1/25#newcode50
> src/routing/click/ipv4-click-routing-test.cc:50: click->SetClickFile
> ("nsclick-simple-lan.click");
> will this path work, or do you need to specify examples/...
> 
> http://codereview.appspot.com/1690056/diff/1/29
> File src/routing/click/ipv4-external-routing.h (right):
> 
> http://codereview.appspot.com/1690056/diff/1/29#newcode31
> src/routing/click/ipv4-external-routing.h:31: class Ipv4ExternalRouting :
public
> Ipv4RoutingProtocol
> maybe a description here about why you need this class
> 
> http://codereview.appspot.com/1690056/diff/1/32
> File src/routing/olsr/olsr-routing-protocol.h (left):
> 
> http://codereview.appspot.com/1690056/diff/1/32#oldcode179
> src/routing/olsr/olsr-routing-protocol.h:179: virtual bool RouteInput 
> (Ptr<const Packet> p, const Ipv4Header &header, Ptr<const NetDevice> idev,
> dont need this

Thanks for the review. Most of these are silly errors that crept in when I'd
made some last minute changes. I'll work on it.

[Lalith Suresh, 2010-08-03 07:17:11]

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 21:51:32, Josh Pelkey wrote:
> I get this error when I run this example (something similar happens to the
other
> examples): 
> 
> [...]

This warning occurs only in the latest git clone, and not in the stable releases
of click (1.8.0 being the latest). I'm not sure why it occurs, (it happens with
the original nsclick from ns-2 as well). And it doesn't seem to be affecting the
simulations either.

[Manuel Requena, 2010-08-05 10:38:10]

Lalith,

Great job! Your work will be very useful for ns-3 and click users.

I send you a quick review.

Manuel

http://codereview.appspot.com/1690056/diff/1/2
File examples/click/nsclick-ip-router.click (right):

http://codereview.appspot.com/1690056/diff/1/2#newcode82
examples/click/nsclick-ip-router.click:82: u :: IPRouter(eth0,eth0,eth1,eth1);
Same warning:

examples/click/nsclick-ip-router.click:31: While configuring ‘u/ar0 ::
ARPResponder’:
  warning: multiple entries for 172.16.1.2/32
examples/click/nsclick-ip-router.click:42: While configuring ‘u/ar1 ::
ARPResponder’:
  warning: multiple entries for 172.16.2.1/32

http://codereview.appspot.com/1690056/diff/1/4
File examples/click/nsclick-raw-wlan.click (right):

http://codereview.appspot.com/1690056/diff/1/4#newcode25
examples/click/nsclick-raw-wlan.click:25: // nsclick-raw-wlan.click
Errors:
examples/click/nsclick-raw-wlan.click:49: unknown element class ‘ExtraDecap’
examples/click/nsclick-raw-wlan.click:51: unknown element class ‘WifiDecap’
examples/click/nsclick-raw-wlan.click: errors prevent router from initializing

You need "--enable-wifi" when configuring Click. I know that this is Click
specific but you should mention something in the wiki/docs:
http://www.nsnam.org/wiki/index.php/GSOC2010Click

http://codereview.appspot.com/1690056/diff/1/5
File examples/click/nsclick-routing-node0.click (right):

http://codereview.appspot.com/1690056/diff/1/5#newcode96
examples/click/nsclick-routing-node0.click:96: u :: DumbRouter(eth0,eth0);
Same warning:

examples/click/nsclick-routing-node0.click:37: While configuring
‘u/myarpresponder :: ARPResponder’:
  warning: multiple entries for 172.16.1.1/32

http://codereview.appspot.com/1690056/diff/1/6
File examples/click/nsclick-routing-node2.click (right):

http://codereview.appspot.com/1690056/diff/1/6#newcode96
examples/click/nsclick-routing-node2.click:96: u :: DumbRouter(eth0,eth0);
Same warning:

examples/click/nsclick-routing-node2.click:37: While configuring
‘u/myarpresponder :: ARPResponder’:
  warning: multiple entries for 172.16.2.2/32

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

http://codereview.appspot.com/1690056/diff/1/8#newcode19
examples/click/nsclick-simple-lan.cc:19: // Scenario:
Could you put IP addresses and device names in the picture? It would help to
read the ns-3 and click configuration files

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

http://codereview.appspot.com/1690056/diff/1/9#newcode97
examples/click/nsclick-simple-lan.click:97: u :: DumbRouter(eth0,eth0);
I get the following:
examples/click/nsclick-simple-lan.click:37: While configuring ‘u/myarpresponder
:: ARPResponder’:
  warning: multiple entries for 172.16.1.1/32

In Click, you can use "eth0:ip" and "eth0:mac". I assume eth0 contains IP+MAC
addresses.
By the way, where "eth0" and "tap0" devices are created? They are created by the
ns-3 core? I can use a different name for the "tap0" device?

http://codereview.appspot.com/1690056/diff/1/13
File src/devices/csma/csma-net-device.cc (right):

http://codereview.appspot.com/1690056/diff/1/13#newcode779
src/devices/csma/csma-net-device.cc:779: m_rxCallback (this,
originalPacket->Copy (), protocol, header.GetSource ());
Why Click needs another copy of the packet? What happens to "packet"? If Click
needs the original packet (with headers) why you don't pass "originalPacket"?

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

http://codereview.appspot.com/1690056/diff/1/24#newcode44
src/routing/click/click.h:44: *   $: ./configure --enable-userlevel
--disable-linuxmodule --enable-nsclick
Add here "--enable-wifi", if not one example won't run

[Lalith Suresh, 2010-08-05 15:01:15]

Hello Manuel,

Thanks for the review! I've incorporated the following changes as you'd
suggested:

1) Click Warnings have been fixed.
2) Wiki has been updated regarding the use of --enable-wifi. So is
src/routing/click/click.h.
3) originalPacket->Copy() has been replaced with originalPacket.
4) Details of IP addresses and device names have been added to the example file
scenario descriptions.

The new patchset has been uploaded. As Josh suggested over IRC, I'll try and get
a complete patch set up all over again.

Regarding your query about the devices "eth0" and "tap0", I followed the same
approach as was done in the original nsclick (ns-2). The device names map are
hard coded in the code as:
- "tapX" or "tunX" for the Kernel tap device, that is, for sending packets to
and from the OS.
- "ethX" which refers to the network devices, wired or wireless, wherein X (the
numbering) starts from 0 onwards.

If you need to use different names, you'll need to edit
Ipv4ClickRouting::GetInterfaceId() to your requirement. If this is an important
need for Click users, I can try and figure out a way to keep the names
changeable from the simulation file?

Thanks again!

Regards,
Lalith

[Tom Henderson, 2010-08-11 23:56:39]

Overall this looks very nice but I had some suggestions for a few issues
(documentation, tests, how it integrates with the ns-3 devices).  See inline.

- Tom

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

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

http://codereview.appspot.com/1690056/diff/1/14#newcode30
src/devices/wifi/mac-low.cc:30: #include "ns3/ipv4-l3-click-protocol.h"
IMO we can't introduce dependencies on Click on the wifi or csma code.  We need
a more general solution.

http://codereview.appspot.com/1690056/diff/1/14#newcode890
src/devices/wifi/mac-low.cc:890: NetDevice::PacketType(0));
We'll need to consult the wifi maintainers about a way to better support 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
Is this the right place for documentation of transport protocol tracing?

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);
Why is this needed?  In what cases would you set to false and then run
Install()?

http://codereview.appspot.com/1690056/diff/1/20
File src/internet-stack/tcp-l4-protocol.cc (right):

http://codereview.appspot.com/1690056/diff/1/20#newcode399
src/internet-stack/tcp-l4-protocol.cc:399: }
I am OK with this for now; it provides yet another reason to find a bug 967
solution.

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

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

http://codereview.appspot.com/1690056/diff/1/25#newcode154
src/routing/click/ipv4-click-routing-test.cc:154: // NS_TEST_EXPECT_MSG_EQ (buf,
NULL, "No eth2");
Not sure either-- we need some kind of "bad input" test capability.  Suggest to
leave it like this for 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#newcode41
src/routing/click/ipv4-click-routing.cc:41: NS_LOG_COMPONENT_DEFINE
("Ipv4ClickRouting");
Define this outside the namespace ns3

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
What are the implications of feeding dummy data to these fields?  Will this
break some click components?

http://codereview.appspot.com/1690056/diff/1/26#newcode433
src/routing/click/ipv4-click-routing.cc:433: {
indentation (should you run your files through check-style.py?)

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')
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?

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

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: */
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?

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

http://codereview.appspot.com/1690056/diff/26001/27001#newcode67
doc/manual/click.texi:67: 
Again, please list here the click elements that work, and the ones that do not
work.  How does the program fail if an unsupported element is used?

Please mention that IPv6 is unsupported (I guess that will be clear in answering
the above list of supported elements).

http://codereview.appspot.com/1690056/diff/26001/27001#newcode123
doc/manual/click.texi:123: @end verbatim

Does click itself produce output (counters, etc.)?  Can you point users to how
they can get data out of Click itself?

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

http://codereview.appspot.com/1690056/diff/26001/27001#newcode171
doc/manual/click.texi:171: @end enumerate
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?

http://codereview.appspot.com/1690056/diff/26001/27001#newcode182
doc/manual/click.texi:182: @end enumerate
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?

[Lalith Suresh, 2010-08-12 07:05:54]

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.