Code review - Issue 1690056: NS-3 Click Integration Code Reviewhttps://codereview.appspot.com/2010-09-02T08:38:03+00:00rietveld
Message from unknown
2010-07-20T18:40:56+00:00Lalith Sureshurn:md5:89f4ec2d13c9b4756a64e0c830d42410
Message from joshpelkey@gmail.com
2010-08-02T21:51:31+00:00Josh Pelkeyurn:md5:42b454118f5508ff9bdf7a0f1b3cdc19
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
Message from suresh.lalith@gmail.com
2010-08-03T05:39:30+00:00Lalith Sureshurn:md5:ada87f08ed685f4497aad6ef875cfe46
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.
Message from unknown
2010-08-03T06:42:36+00:00Lalith Sureshurn:md5:3b36a7374260d21fa885aef86f5ef269
Message from suresh.lalith@gmail.com
2010-08-03T07:17:11+00:00Lalith Sureshurn:md5:af79ea0a273dbbc286de1e53b446dc77
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.
Message from manuel.requena@gmail.com
2010-08-05T10:38:10+00:00Manuel Requenaurn:md5:afe99bd3f0cc4462ebb5e323b4e68db8
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
Message from unknown
2010-08-05T14:45:36+00:00Lalith Sureshurn:md5:c6e1f5a2c37975f5b32d26e2e3e6ff79
Message from suresh.lalith@gmail.com
2010-08-05T15:01:15+00:00Lalith Sureshurn:md5:95c5415954aab920750ce6a9666658da
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
Message from unknown
2010-08-11T12:33:57+00:00Lalith Sureshurn:md5:c8d829bf17356fea825d74ad19d52f84
Message from tomh.org@gmail.com
2010-08-11T23:56:39+00:00Tom Hendersonurn:md5:315ba87c4f447929fcd990147de61ee7
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?
Message from unknown
2010-08-12T07:01:18+00:00Lalith Sureshurn:md5:9984ff81e5b406ca47a9ab261ebf4aa9
Message from suresh.lalith@gmail.com
2010-08-12T07:05:54+00:00Lalith Sureshurn:md5:d614215c7a872a60c6186e2fa5a5a03d
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.
Message from unknown
2010-08-13T07:54:50+00:00Lalith Sureshurn:md5:5637ce56524e8431dd0735750906f054
Message from unknown
2010-09-02T08:38:03+00:00Lalith Sureshurn:md5:b3f16bf61017066701b13d8b28a0dbee