Code review - Issue 45320043: New Simple applicationshttps://codereview.appspot.com/2014-09-09T20:36:29+00:00rietveld
Message from unknown
2013-12-24T08:57:48+00:00Tommaso Pecorellaurn:md5:be3f2a846d32e4a55aab4a0b8b70f1ff
Message from unknown
2014-04-05T20:05:36+00:00Tommaso Pecorellaurn:md5:fb6ad52075b69d97abbde2f6d80610fc
Message from tommypec@gmail.com
2014-04-05T20:05:53+00:00Tommaso Pecorellaurn:md5:9ecc70e5a01b73d832a4fe42770a9649
Message from tommypec@gmail.com
2014-04-05T20:40:01+00:00Tommaso Pecorellaurn:md5:e17329a9a548137e1d2aeba0dcce2c15
Test and example application added.
Valgrind tests are fine.
T.
Message from tomh.org@gmail.com
2014-04-11T13:51:28+00:00Tom Hendersonurn:md5:1c3dddfe2fba57d8d0641c3a4888b63c
My main comment is that it isn't clear to me whether this will work on a node with multiple net devices and how multiple packet flows are successfully demultiplexed at the receiver. Probably more API is needed. Here is a challenge test for the test suite: create a client node with two SimpleNetDevices A and B, and two send apps As and Bs. Create a server node with two SimpleNetDevices C and D, and two server apps Cr and Dr. Connect A<->C and B<->D. Have app As send some number of packets to Cr, and Bs to Dr. Verify that Cr gets only As packets, and Dr only Bs packets.
Regarding the class naming, elsewhere in ns-3 we spell out "Application" in the class name, so I'd prefer to either spell it out here or find different names. How about "SimpleClient" and "SimpleServer" names instead?
Finally, the class doesn't seem to easily allow for a user to send packets at arbitrary times, only regular intervals starting at time zero. For instance, what if I just wanted to send packets only at times 3, 10, and 30 seconds-- how could I do that?
Other comments are interleaved in the code.
https://codereview.appspot.com/45320043/diff/20001/src/network/examples/simple-apps.cc
File src/network/examples/simple-apps.cc (right):
https://codereview.appspot.com/45320043/diff/20001/src/network/examples/simple-apps.cc#newcode24
src/network/examples/simple-apps.cc:24: using namespace ns3;
needs documentation about what this example tries to demonstrate
https://codereview.appspot.com/45320043/diff/20001/src/network/examples/simple-apps.cc#newcode70
src/network/examples/simple-apps.cc:70: socketAddr.SetProtocol (1);
document where the value "1" comes from
https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-recv-app.cc
File src/network/utils/simple-recv-app.cc (right):
https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-recv-app.cc#newcode107
src/network/utils/simple-recv-app.cc:107: { //EOF
add debug log statement here
https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-recv-app.cc#newcode112
src/network/utils/simple-recv-app.cc:112: if (PacketSocketAddress::IsMatchingType (from))
You are incrementing the counters and firing the trace source even if the packet fails this test?
https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.cc
File src/network/utils/simple-send-app.cc (right):
https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.cc#newcode52
src/network/utils/simple-send-app.cc:52: MakeUintegerAccessor (&SimpleSendApp::m_count),
this variable name would be better named m_maxpackets because it is not a count (m_sent is the count)
https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.cc#newcode64
src/network/utils/simple-send-app.cc:64: "Size of packets generated.",
specify "(bytes)"
https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.cc#newcode68
src/network/utils/simple-send-app.cc:68: ;
suggest to add Tx trace source
https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.cc#newcode127
src/network/utils/simple-send-app.cc:127: m_sendEvent = Simulator::Schedule (Seconds (0.0), &SimpleSendApp::Send, this);
do you think users will always want to start this at time zero? Should this value be configurable?
https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.h
File src/network/utils/simple-send-app.h (right):
https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.h#newcode39
src/network/utils/simple-send-app.h:39: * Sends packets using PacketSocket. It does not require (or use) IP.
more details about the attributes (and trace sources) here, please.
Document also that it is intended to be used for SimpleNetDevice or devices that support the PacketSocket interface (which assumes Mac48Address).
Document what will happen if user sets packet size to exceed the device's MTU.
Message from unknown
2014-04-12T12:44:54+00:00Tommaso Pecorellaurn:md5:947bafb7e56a7d9823ad6948faced859
Message from tommypec@gmail.com
2014-04-12T12:44:59+00:00Tommaso Pecorellaurn:md5:d93984c24e22cbbec5334bb5a28bfdc1
Message from unknown
2014-04-12T12:58:15+00:00Tommaso Pecorellaurn:md5:7304a074b5998611e381be38eaac9a0c
Message from tommypec@gmail.com
2014-04-12T12:58:16+00:00Tommaso Pecorellaurn:md5:f51619faf432b261261448a7c99fe173
Message from tommypec@gmail.com
2014-04-12T12:58:32+00:00Tommaso Pecorellaurn:md5:a640c25c378df17aca24729c5d83e00c
On 2014/04/11 13:51:28, Tom Henderson wrote:
> My main comment is that it isn't clear to me whether this will work on a node
> with multiple net devices and how multiple packet flows are successfully
> demultiplexed at the receiver. Probably more API is needed. Here is a
> challenge test for the test suite: create a client node with two
> SimpleNetDevices A and B, and two send apps As and Bs. Create a server node
> with two SimpleNetDevices C and D, and two server apps Cr and Dr. Connect A<->C
> and B<->D. Have app As send some number of packets to Cr, and Bs to Dr. Verify
> that Cr gets only As packets, and Dr only Bs packets.
It will not work as you think. The point is: there's no L4 protocol in between a PacketSocket
and a SimpleNetDevice. As a consequence, there's no mux/demux. The only mux/demux is
done by using the protocol field (if the NetDevice supports it).
> Regarding the class naming, elsewhere in ns-3 we spell out "Application" in the
> class name, so I'd prefer to either spell it out here or find different names.
> How about "SimpleClient" and "SimpleServer" names instead?
I renamed them.
> Finally, the class doesn't seem to easily allow for a user to send packets at
> arbitrary times, only regular intervals starting at time zero. For instance,
> what if I just wanted to send packets only at times 3, 10, and 30 seconds-- how
> could I do that?
You can't :)
About the staring time, just change the Application::Start() time. About sending
packets at specific times, it's not supported.
Mind that the apps are meant to be used in tests, and usually a single packet is more
than enough.
> Other comments are interleaved in the code.
>
> https://codereview.appspot.com/45320043/diff/20001/src/network/examples/simple-apps.cc
> File src/network/examples/simple-apps.cc (right):
>
> https://codereview.appspot.com/45320043/diff/20001/src/network/examples/simple-apps.cc#newcode24
> src/network/examples/simple-apps.cc:24: using namespace ns3;
> needs documentation about what this example tries to demonstrate
Done.
> https://codereview.appspot.com/45320043/diff/20001/src/network/examples/simple-apps.cc#newcode70
> src/network/examples/simple-apps.cc:70: socketAddr.SetProtocol (1);
> document where the value "1" comes from
It's arbitrary, Documented.
> https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-recv-app.cc
> File src/network/utils/simple-recv-app.cc (right):
>
> https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-recv-app.cc#newcode107
> src/network/utils/simple-recv-app.cc:107: { //EOF
> add debug log statement here
To be honest, it was a totally useless check. Removed.
> https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-recv-app.cc#newcode112
> src/network/utils/simple-recv-app.cc:112: if
> (PacketSocketAddress::IsMatchingType (from))
> You are incrementing the counters and firing the trace source even if the packet
> fails this test?
Fixed.
> https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.cc
> File src/network/utils/simple-send-app.cc (right):
>
> https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.cc#newcode52
> src/network/utils/simple-send-app.cc:52: MakeUintegerAccessor
> (&SimpleSendApp::m_count),
> this variable name would be better named m_maxpackets because it is not a count
> (m_sent is the count)
Renamed.
> https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.cc#newcode64
> src/network/utils/simple-send-app.cc:64: "Size of packets generated.",
> specify "(bytes)"
Done.
> https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.cc#newcode68
> src/network/utils/simple-send-app.cc:68: ;
> suggest to add Tx trace source
Done.
> https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.cc#newcode127
> src/network/utils/simple-send-app.cc:127: m_sendEvent = Simulator::Schedule
> (Seconds (0.0), &SimpleSendApp::Send, this);
> do you think users will always want to start this at time zero? Should this
> value be configurable?
See above.
> https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.h
> File src/network/utils/simple-send-app.h (right):
>
> https://codereview.appspot.com/45320043/diff/20001/src/network/utils/simple-send-app.h#newcode39
> src/network/utils/simple-send-app.h:39: * Sends packets using PacketSocket. It
> does not require (or use) IP.
> more details about the attributes (and trace sources) here, please.
Done.
> Document also that it is intended to be used for SimpleNetDevice or devices that
> support the PacketSocket interface (which assumes Mac48Address).
Done
> Document what will happen if user sets packet size to exceed the device's MTU.
Done.
Message from tomh.org@gmail.com
2014-04-12T23:30:50+00:00Tom Hendersonurn:md5:ee15a9454f10efeb3b670351c8776ea4
On 2014/04/12 12:58:32, Tommaso Pecorella wrote:
> On 2014/04/11 13:51:28, Tom Henderson wrote:
> > My main comment is that it isn't clear to me whether this will work on a node
> > with multiple net devices and how multiple packet flows are successfully
> > demultiplexed at the receiver. Probably more API is needed. Here is a
> > challenge test for the test suite: create a client node with two
> > SimpleNetDevices A and B, and two send apps As and Bs. Create a server node
> > with two SimpleNetDevices C and D, and two server apps Cr and Dr. Connect
> A<->C
> > and B<->D. Have app As send some number of packets to Cr, and Bs to Dr.
> Verify
> > that Cr gets only As packets, and Dr only Bs packets.
>
> It will not work as you think. The point is: there's no L4 protocol in between a
> PacketSocket
> and a SimpleNetDevice. As a consequence, there's no mux/demux. The only
> mux/demux is
> done by using the protocol field (if the NetDevice supports it).
This reply made me realize that I'm unsure about what this application is supposed to be used for. It seems limited to only providing a packet generator to sit directly on top of SimpleNetDevices, in a single interface/channel configuration, which doesn't seem to be of much practical use.
Could you clarify existing tests and examples that you would insert this application into, to replace any existing packet generation code?
I had thought that the main issue being addressed was that there were no applications written that didn't require dependency on the internet module (which is pulled in by the src/applications module), so this application could be used to directly test devices such as Csma.
Message from unknown
2014-04-13T11:22:02+00:00Tommaso Pecorellaurn:md5:7b4328024b0332cbf33d76eca20e44e2
Message from tommypec@gmail.com
2014-04-13T11:22:05+00:00Tommaso Pecorellaurn:md5:f17381fe02797f249f892aaed20db3de
Message from tommypec@gmail.com
2014-04-13T11:26:46+00:00Tommaso Pecorellaurn:md5:7510a37e373e36ac3553abafbbeaced6
On 2014/04/12 23:30:50, Tom Henderson wrote:
> On 2014/04/12 12:58:32, Tommaso Pecorella wrote:
> > On 2014/04/11 13:51:28, Tom Henderson wrote:
> > > My main comment is that it isn't clear to me whether this will work on a
> node
> > > with multiple net devices and how multiple packet flows are successfully
> > > demultiplexed at the receiver. Probably more API is needed. Here is a
> > > challenge test for the test suite: create a client node with two
> > > SimpleNetDevices A and B, and two send apps As and Bs. Create a server node
> > > with two SimpleNetDevices C and D, and two server apps Cr and Dr. Connect
> > A<->C
> > > and B<->D. Have app As send some number of packets to Cr, and Bs to Dr.
> > Verify
> > > that Cr gets only As packets, and Dr only Bs packets.
> >
> > It will not work as you think. The point is: there's no L4 protocol in between
> a
> > PacketSocket
> > and a SimpleNetDevice. As a consequence, there's no mux/demux. The only
> > mux/demux is
> > done by using the protocol field (if the NetDevice supports it).
>
>
> This reply made me realize that I'm unsure about what this application is
> supposed to be used for. It seems limited to only providing a packet generator
> to sit directly on top of SimpleNetDevices, in a single interface/channel
> configuration, which doesn't seem to be of much practical use.
>
> Could you clarify existing tests and examples that you would insert this
> application into, to replace any existing packet generation code?
>
> I had thought that the main issue being addressed was that there were no
> applications written that didn't require dependency on the internet module
> (which is pulled in by the src/applications module), so this application could
> be used to directly test devices such as Csma.
Not really only on top of SimpleNetDevices. On top of anything that can be used with a PacketSocket.
The real reason I did this is to remove the dependency between the wave module and the application module, which was needed because the wave test used OnOff application....
Limited ? Yes.
Still useful ? I think so.
Message from tomh.org@gmail.com
2014-04-13T18:19:32+00:00Tom Hendersonurn:md5:efd3e80495c428ee84039dd27177e27d
On 2014/04/13 11:26:46, Tommaso Pecorella wrote:
> On 2014/04/12 23:30:50, Tom Henderson wrote:
> > On 2014/04/12 12:58:32, Tommaso Pecorella wrote:
> > > On 2014/04/11 13:51:28, Tom Henderson wrote:
> > > > My main comment is that it isn't clear to me whether this will work on a
> > node
> > > > with multiple net devices and how multiple packet flows are successfully
> > > > demultiplexed at the receiver. Probably more API is needed. Here is a
> > > > challenge test for the test suite: create a client node with two
> > > > SimpleNetDevices A and B, and two send apps As and Bs. Create a server
> node
> > > > with two SimpleNetDevices C and D, and two server apps Cr and Dr. Connect
> > > A<->C
> > > > and B<->D. Have app As send some number of packets to Cr, and Bs to Dr.
> > > Verify
> > > > that Cr gets only As packets, and Dr only Bs packets.
> > >
> > > It will not work as you think. The point is: there's no L4 protocol in
> between
> > a
> > > PacketSocket
> > > and a SimpleNetDevice. As a consequence, there's no mux/demux. The only
> > > mux/demux is
> > > done by using the protocol field (if the NetDevice supports it).
> >
> >
> > This reply made me realize that I'm unsure about what this application is
> > supposed to be used for. It seems limited to only providing a packet
> generator
> > to sit directly on top of SimpleNetDevices, in a single interface/channel
> > configuration, which doesn't seem to be of much practical use.
> >
> > Could you clarify existing tests and examples that you would insert this
> > application into, to replace any existing packet generation code?
> >
> > I had thought that the main issue being addressed was that there were no
> > applications written that didn't require dependency on the internet module
> > (which is pulled in by the src/applications module), so this application could
> > be used to directly test devices such as Csma.
>
> Not really only on top of SimpleNetDevices. On top of anything that can be used
> with a PacketSocket.
>
> The real reason I did this is to remove the dependency between the wave module
> and the application module, which was needed because the wave test used OnOff
> application....
> Limited ? Yes.
> Still useful ? I think so.
I agree with the use case you suggest, and there are more of these. But in this case, should it be instead called something like "PacketSocketClient" and "PacketSocketServer" since it is restricted in scope to PacketSocket (i.e. naming of "Simple" seems to associate it with "SimpleNetDevice"). And if so, do my previous comments apply (that it would be useful to be able to bind the sender and receiver when the underlying device is not the SimpleNetDevice)?
Message from tommypec@gmail.com
2014-04-13T18:21:47+00:00Tommaso Pecorellaurn:md5:8f5e3b9713960f6412089fd0ad7ef52b
On 2014/04/13 18:19:32, Tom Henderson wrote:
> >
> > The real reason I did this is to remove the dependency between the wave module
> > and the application module, which was needed because the wave test used OnOff
> > application....
> > Limited ? Yes.
> > Still useful ? I think so.
>
>
> I agree with the use case you suggest, and there are more of these. But in this
> case, should it be instead called something like "PacketSocketClient" and
> "PacketSocketServer" since it is restricted in scope to PacketSocket (i.e.
> naming of "Simple" seems to associate it with "SimpleNetDevice"). And if so, do
> my previous comments apply (that it would be useful to be able to bind the
> sender and receiver when the underlying device is not the SimpleNetDevice)?
I've always been bad at naming stuff... consider the renaming done.
Cheers,
T.
Message from unknown
2014-04-13T18:55:04+00:00Tommaso Pecorellaurn:md5:1776de65c896590b7075e6e180520154
Message from tommypec@gmail.com
2014-04-13T18:55:22+00:00Tommaso Pecorellaurn:md5:361461c797ad1f8d0a778aecf1b209a2
Message from unknown
2014-04-19T10:07:41+00:00Tommaso Pecorellaurn:md5:751c065a0d7c143f336df46aff26e9ec
Message from tommypec@gmail.com
2014-04-19T10:07:43+00:00Tommaso Pecorellaurn:md5:e8eec96965ab9c1d533d1eccff9b76a9
Message from unknown
2014-07-27T21:54:53+00:00Tommaso Pecorellaurn:md5:a4bb5319e7a108490e72c78eefa317f0
Message from tommypec@gmail.com
2014-07-27T21:54:58+00:00Tommaso Pecorellaurn:md5:14e6f3498daa35b9bf38a1dc04b8e245
Message from tomh.org@gmail.com
2014-09-09T20:36:29+00:00Tom Hendersonurn:md5:ba8adf46b162a35b597a1f3cb93c7d49
At this point, just a couple trivial comments; I think this is ready for merge.
https://codereview.appspot.com/45320043/diff/120001/src/network/test/simple-apps-test-suite.cc
File src/network/test/simple-apps-test-suite.cc (right):
https://codereview.appspot.com/45320043/diff/120001/src/network/test/simple-apps-test-suite.cc#newcode23
src/network/test/simple-apps-test-suite.cc:23: #include "ns3/network-module.h"
do not use "-module" variants here
https://codereview.appspot.com/45320043/diff/120001/src/network/test/simple-apps-test-suite.cc#newcode108
src/network/test/simple-apps-test-suite.cc:108: Simulator::Destroy ();
redundant Destroy() statements.
https://codereview.appspot.com/45320043/diff/120001/src/network/utils/packet-socket-client.cc
File src/network/utils/packet-socket-client.cc (right):
https://codereview.appspot.com/45320043/diff/120001/src/network/utils/packet-socket-client.cc#newcode100
src/network/utils/packet-socket-client.cc:100: NS_ASSERT_MSG (m_peerAddressSet, "Local address not set");
peer address not set