fd-net-device: a module to handle interconnection between ns-3 simulation and the 'outside world' i…

fd-net-device: a module to handle interconnection between ns-3 simulation and the 'outside world' in a flexible way.

[Tom Henderson, 2012-08-31 23:25:25]

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/examples/fd-em...
File src/fd-net-device/examples/fd-emu-udp-echo.cc (right):

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/examples/fd-em...
src/fd-net-device/examples/fd-emu-udp-echo.cc:56: //             "eth1"
this diagram doesn't correspond to the example (it is broken upstream, and
should be limited to n0 and n1) but anyway, this is a contrived example.

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/examples/fd-em...
src/fd-net-device/examples/fd-emu-udp-echo.cc:115: EmuFdNetDeviceHelper emu;
I originally thought it might be better to preserve "EmuHelper" here, because
"EmuFdNetDeviceHelper" seems unnecessarily wordy.  However, it seems to me that
it maybe doesn't matter, and we might get into subtle failures if the helper is
not renamed.  e.g. emu.SetAttribute ("DeviceName") will have no effect, since
SetDeviceName() must instead be called.  Any other opinions?

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/helper/planetl...
File src/fd-net-device/helper/planetlab-fd-net-device-helper.h (right):

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/helper/planetl...
src/fd-net-device/helper/planetlab-fd-net-device-helper.h:66: void SetTapMask
(Ipv4Mask mask);

IPv6 also?

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/helper/tap-fd-...
File src/fd-net-device/helper/tap-fd-net-device-helper.h (right):

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/helper/tap-fd-...
src/fd-net-device/helper/tap-fd-net-device-helper.h:64: void SetTapIpAddress
(Ipv4Address address);
SetTapIpv4Address 

(and consider to add the Ipv6 variants)

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/helper/tap-fd-...
src/fd-net-device/helper/tap-fd-net-device-helper.h:71: void SetTapMask
(Ipv4Mask mask);
SetTapIpv4Mask

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/model/fd-net-d...
File src/fd-net-device/model/fd-net-device.cc (right):

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/model/fd-net-d...
src/fd-net-device/model/fd-net-device.cc:164: NS_LOG_FUNCTION_NOARGS ();
NS_LOG_FUNCTION (this);

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/model/fd-net-d...
src/fd-net-device/model/fd-net-device.cc:183: return;
is this test unnecessary?  (values constrained already by the enum)

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/model/fd-net-d...
src/fd-net-device/model/fd-net-device.cc:193: NS_LOG_FUNCTION_NOARGS ();
NS_LOG_FUNCTION (this);

Strictly speaking, these setters/getters are not necessary due to the
attributes, but perhaps this is easier syntax for users.

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/model/fd-net-d...
src/fd-net-device/model/fd-net-device.cc:216: NS_LOG_FUNCTION_NOARGS ();
NS_LOG_FUNCTION(this);  need to grep for other instances

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/model/fd-net-d...
src/fd-net-device/model/fd-net-device.cc:268: // will be blocked until the
condition is released.
NS_LOG_WARN??  for debugging?

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/model/fd-net-d...
File src/fd-net-device/model/fd-net-device.h (right):

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/model/fd-net-d...
src/fd-net-device/model/fd-net-device.h:76: *    to extend new encapsulation
modes.
I am neutral about this, because I'm not sure what use cases you have in mind
that users will want to make this more flexible.  Can you elaborate?

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/model/fd-net-d...
src/fd-net-device/model/fd-net-device.h:103: // inherited from NetDevice base
class.
move these downward below the FdNetDevice-specific API declarations

http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/model/fd-net-d...
src/fd-net-device/model/fd-net-device.h:165: void Stop (Time tStop);
Are Start/Stop used in practice?  What is the use case?

[aquereilhac, 2012-09-09 17:17:46]

On 2012/08/31 23:25:25, Tom Henderson wrote:
> 
>
http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/examples/fd-em...
> src/fd-net-device/examples/fd-emu-udp-echo.cc:115: EmuFdNetDeviceHelper emu;
> I originally thought it might be better to preserve "EmuHelper" here, because
> "EmuFdNetDeviceHelper" seems unnecessarily wordy.  However, it seems to me
that
> it maybe doesn't matter, and we might get into subtle failures if the helper
is
> not renamed.  e.g. emu.SetAttribute ("DeviceName") will have no effect, since
> SetDeviceName() must instead be called.  Any other opinions?
> 

I agree in that the helper class names are too long. What do you think about
changing them to TAPEmuHelper, NICEmuHelper (for network interface card helper,
instead of EmuFdNetDeviceHelper), and PlanetLabEmuHelper?
In the end they are all 'Emulation helpers'.

 
>
http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/helper/planetl...
> src/fd-net-device/helper/planetlab-fd-net-device-helper.h:66: void SetTapMask
> (Ipv4Mask mask);
> 
> IPv6 also?

PlanetLab doesn't support IPv6 for the moment (neither will in the near future).


>
http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/model/fd-net-d...
> src/fd-net-device/model/fd-net-device.h:76: *    to extend new encapsulation
> modes.
> I am neutral about this, because I'm not sure what use cases you have in mind
> that users will want to make this more flexible.  Can you elaborate?

I don't have any concrete use case for the moment. In the future people might
want to use other encapsulations (i.e. that they developed themselves as part of
their research) and inject traffic generated with these encapsulations. However,
they could as well manipulate the traffic outside of ns-3 and inject already
decapsulated traffic... I'll remove this comment in any case.

>
http://codereview.appspot.com/6458154/diff/1/src/fd-net-device/model/fd-net-d...
> src/fd-net-device/model/fd-net-device.h:165: void Stop (Time tStop);
> Are Start/Stop used in practice?  What is the use case?

This can be useful if the user wants to stop/re-start receiving traffic from the
outside world, it would be equivalent to set a link up/down.
It could be used to emulate connection/disconnection patterns for example.

[aquereilhac, 2013-03-07 14:26:52]

Adding throughput measurement examples for FdNetDevice + performance bug-fix

[aquereilhac, 2013-03-07 15:09:54]

Adding throughput measurement examples for FdNetDevice + performance bug-fix

[Fred, 2013-03-11 16:57:11]

tap-creator.cc already exists under tap-bridge/model

https://codereview.appspot.com/6458154/diff/20001/src/fd-net-device/examples/...
File src/fd-net-device/examples/fd2fd-onoff.cc (right):

https://codereview.appspot.com/6458154/diff/20001/src/fd-net-device/examples/...
src/fd-net-device/examples/fd2fd-onoff.cc:45: // 2 - Run the sexperiment
typo "sexperiment"

https://codereview.appspot.com/6458154/diff/20001/src/fd-net-device/helper/pl...
File src/fd-net-device/helper/planetlab-fd-net-device-helper.h (right):

https://codereview.appspot.com/6458154/diff/20001/src/fd-net-device/helper/pl...
src/fd-net-device/helper/planetlab-fd-net-device-helper.h:39: * \brief build a
set of FdNetDevice objects attached to a virtua TAP network
typo

https://codereview.appspot.com/6458154/diff/20001/src/fd-net-device/helper/ta...
File src/fd-net-device/helper/tap-creator.cc (right):

https://codereview.appspot.com/6458154/diff/20001/src/fd-net-device/helper/ta...
src/fd-net-device/helper/tap-creator.cc:414: // us to exeucte the following
code:
typo

https://codereview.appspot.com/6458154/diff/20001/src/fd-net-device/model/fd-...
File src/fd-net-device/model/fd-net-device.h (right):

https://codereview.appspot.com/6458154/diff/20001/src/fd-net-device/model/fd-...
src/fd-net-device/model/fd-net-device.h:194: * Forward the frame to the
appropiate callback for processing
type "appropiate"

[aquereilhac, 2013-03-12 18:10:02]

Addressing comments from review https://codereview.appspot.com/6458154/#msg5

[aquereilhac, 2013-03-12 22:31:17]

Added validation for device name set on EmuFdNetDeviceHelper::SetFileDescriptor
()

[aquereilhac, 2013-03-15 18:26:47]

Added .rst documentation file provided by Tom + further details on the 'Design'
section

[Tom Henderson, 2013-03-16 14:41:35]

Please add ns-3-reviews as cc to this issue.

I reviewed this earlier to write the draft documentation; today I just left a
few comments that are nits.

Mainly, the documentation chapter needs to be finished off.

Here is the proposed merge plan.  This seems ready to go in once the comments
are addressed.  Then, we can leave in the src/emu module for now, but put a
comment in the documentation here:
http://www.nsnam.org/docs/release/3.16/models/html/emu.html
that the device is deprecated and users should start using the new device
helper.

We also need to fix the wiki pages (HOWTOs).

This chapter probably needs revision/refreshing also:
http://www.nsnam.org/docs/release/3.16/models/html/emulation-overview.html

Anyway, I propose that we merge the code, then fix the documentation before the
release.

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/doc/fd-ne...
File src/fd-net-device/doc/fd-net-device.rst (right):

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/doc/fd-ne...
src/fd-net-device/doc/fd-net-device.rst:128: of RxQueueSize (1000) matter?  If
so, when?
Alina, I think you addressed this above, or else can you elaborate here?

Should we mention the 60 Mb/s limit here?

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/doc/fd-ne...
src/fd-net-device/doc/fd-net-device.rst:146: XXX Alina, can you update the below
as needed, about root privileges?
remove XXX comment

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/doc/fd-ne...
src/fd-net-device/doc/fd-net-device.rst:308: XXX Alina to write
Please address this

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/doc/fd-ne...
src/fd-net-device/doc/fd-net-device.rst:315: * ``Mtu``:  The MAC-level MTU (XXX
guidance to set this from value of zero?)
XXX comment here

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/doc/fd-ne...
src/fd-net-device/doc/fd-net-device.rst:359: XXX Alina do you think there is
anything to say in this section?
remove comment, and fix or delete section

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/doc/fd-ne...
src/fd-net-device/doc/fd-net-device.rst:378: * ``fd-tap6-ping.cc``: Describe
describe the examples.

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/doc/fd-ne...
src/fd-net-device/doc/fd-net-device.rst:390: XXX Alina to write this section.
address comment

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
File src/fd-net-device/examples/dummy-network.cc (right):

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
src/fd-net-device/examples/dummy-network.cc:20: // Network topology
Please add comments about what this example demonstrates.

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
File src/fd-net-device/examples/fd-emu-onoff.cc (right):

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
src/fd-net-device/examples/fd-emu-onoff.cc:68: // server machine: $ ./waf
--run="fd-bw --serverMode=1"
can you describe somewhere what the different serverMode options mean?

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
File src/fd-net-device/examples/fd-emu-ping.cc (right):

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
src/fd-net-device/examples/fd-emu-ping.cc:2: /*
copyright missing

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
File src/fd-net-device/examples/fd-emu-udp-echo.cc (right):

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
src/fd-net-device/examples/fd-emu-udp-echo.cc:2: /*
copyright missing again.  This is ported from Craig Dowell's original example.

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
File src/fd-net-device/examples/fd-planetlab-ping.cc (right):

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
src/fd-net-device/examples/fd-planetlab-ping.cc:2: /*

copyright missing

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
File src/fd-net-device/examples/fd-tap-ping.cc (right):

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
src/fd-net-device/examples/fd-tap-ping.cc:2: /*
copyright missing

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
File src/fd-net-device/examples/fd-tap-ping6.cc (right):

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
src/fd-net-device/examples/fd-tap-ping6.cc:2: /*
copyright missing

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
File src/fd-net-device/examples/wscript (right):

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
src/fd-net-device/examples/wscript:27: obj.source = 'fd-planetlab-ping.cc'
in the documentation, is it clear how each of these build flags are enabled? 
What are the prerequisites, and how can the user check that these flags are set?

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/model/fd-...
File src/fd-net-device/model/fd-net-device.cc (right):

https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/model/fd-...
src/fd-net-device/model/fd-net-device.cc:76: UintegerValue (0),
why is this default 0?  What does this attribute do (when is it set to a valid
value)?

[aquereilhac, 2013-03-17 13:20:00]

Addressing comments from review https://codereview.appspot.com/6458154/#msg9

[aquereilhac, 2013-03-17 13:44:51]

I have partially addressed these comments on Patch Set 6. More detail inline
below:

On 2013/03/16 14:41:35, Tom Henderson wrote:
> Please add ns-3-reviews as cc to this issue.
> 

Seems to be already CC'ed.

> Anyway, I propose that we merge the code, then fix the documentation before
the
> release.
> 

Ok. I will continue to address comments incrementally before the release. What
is the procedure to merge this code into ns-3?

>
https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/doc/fd-ne...
> src/fd-net-device/doc/fd-net-device.rst:128: of RxQueueSize (1000) matter?  If
> so, when?
> Alina, I think you addressed this above, or else can you elaborate here?
> 
> Should we mention the 60 Mb/s limit here?

Added more information about these two point on the "Scope and Limitations"
section.

> 
>
https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/doc/fd-ne...
> src/fd-net-device/doc/fd-net-device.rst:146: XXX Alina, can you update the
below
> as needed, about root privileges?
> remove XXX comment
> 

Added comment about root privileges. Still need to update this section.

>
https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/doc/fd-ne...
> src/fd-net-device/doc/fd-net-device.rst:315: * ``Mtu``:  The MAC-level MTU
(XXX
> guidance to set this from value of zero?)
> XXX comment here
> 

I removed the MTU as a TypeId attribute of the FdNetDevice. The MTU of the
device should reflect that of the network interface associated to the file
descriptor. Changing a MTU attribute on the FdNetDevice will not affect the
associated network interface, so it might be confusing to expose to the user the
possibility of modifying the MTU of the FdNetDevice through the TypeId system.
Rather, it is the helpers who are responsible to set the correct MTU value. I
added this information in the documentation.


>
https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/doc/fd-ne...
> src/fd-net-device/doc/fd-net-device.rst:359: XXX Alina do you think there is
> anything to say in this section?
> remove comment, and fix or delete section
> 

Section removed.

>
https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
> src/fd-net-device/examples/fd-emu-ping.cc:2: /*
> copyright missing
>

Copyright added.
 
 
>
https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
> src/fd-net-device/examples/fd-emu-udp-echo.cc:2: /*
> copyright missing again.  This is ported from Craig Dowell's original example.
> 

Copyright added. Original example doesn't have authors, should I add Craig as
author to this file?

>
https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
> src/fd-net-device/examples/fd-planetlab-ping.cc:2: /*
> 
> copyright missing

Copyright added.

> 
>
https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
> src/fd-net-device/examples/fd-tap-ping.cc:2: /*
> copyright missing

Copyright added.
 
>
https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/examples/...
> src/fd-net-device/examples/fd-tap-ping6.cc:2: /*
> copyright missing

Copyright added.

>
https://codereview.appspot.com/6458154/diff/34001/src/fd-net-device/model/fd-...
> src/fd-net-device/model/fd-net-device.cc:76: UintegerValue (0),
> why is this default 0?  What does this attribute do (when is it set to a valid
> value)?

Now internal MTU takes by default 1500. Another value can be set by the helpers
upon creation of the file descriptor.

[Hajime Tazaki, 2013-03-22 13:05:31]

I've reviewed through helper/model codes and am mostly fine.
A couple of minor comments left are not critical.

https://codereview.appspot.com/6458154/diff/43001/src/fd-net-device/model/fd-...
File src/fd-net-device/model/fd-net-device.cc (right):

https://codereview.appspot.com/6458154/diff/43001/src/fd-net-device/model/fd-...
src/fd-net-device/model/fd-net-device.cc:265: //std::cerr << std::endl <<
"EmuNetDevice read thread: m_pendingReadCount is " << m_pendingReadCount <<
std::endl;
remove unused comment

https://codereview.appspot.com/6458154/diff/43001/src/fd-net-device/model/fd-...
src/fd-net-device/model/fd-net-device.cc:279: struct timespec time = { 0,
100000000L }; // 100 ms
i'm not sure whether this hard-coded timeout value is appropriate or not. using
an attribute?

[aquereilhac, 2013-03-27 00:28:45]

On 2013/03/22 13:05:31, Hajime Tazaki wrote:
> I've reviewed through helper/model codes and am mostly fine.
> A couple of minor comments left are not critical.
> 
>
https://codereview.appspot.com/6458154/diff/43001/src/fd-net-device/model/fd-...
> File src/fd-net-device/model/fd-net-device.cc (right):
> 
>
https://codereview.appspot.com/6458154/diff/43001/src/fd-net-device/model/fd-...
> src/fd-net-device/model/fd-net-device.cc:265: //std::cerr << std::endl <<
> "EmuNetDevice read thread: m_pendingReadCount is " << m_pendingReadCount <<
> std::endl;
> remove unused comment
> 

Ok. I'll modify this directly when I do the merge.

>
https://codereview.appspot.com/6458154/diff/43001/src/fd-net-device/model/fd-...
> src/fd-net-device/model/fd-net-device.cc:279: struct timespec time = { 0,
> 100000000L }; // 100 ms
> i'm not sure whether this hard-coded timeout value is appropriate or not.
using
> an attribute?

I am not sure it would make sense to let users modify this parameter... this
nano sleep occurs when the FdNetDevice is 'saturated' and can't handle any more
packets. If no sleep is done there (or it is too small) the next packets will be
dropped as well. So the nano sleep serves to let packets be processed before the
next read. If on the contrary the sleep is too big the performance of the device
will be unnecessarily low.