Code review - Issue 2408042: Ns-3 BulkSendApplicationhttps://codereview.appspot.com/2010-12-09T16:42:17+00:00rietveld
Message from unknown
2010-10-12T16:46:33+00:00Josh Pelkeyurn:md5:a757b3366295803f149afc22d41be085
Message from joshpelkey@gmail.com
2010-10-12T16:50:31+00:00Josh Pelkeyurn:md5:6c509e5f83480b074653538f576e6807
Also plan adding a test case to src/application/bulk-send-test-case.cc once we figure out the UDP issue.
http://codereview.appspot.com/2408042/diff/1/examples/udp/udp-bulk-send.cc
File examples/udp/udp-bulk-send.cc (right):
http://codereview.appspot.com/2408042/diff/1/examples/udp/udp-bulk-send.cc#newcode1
examples/udp/udp-bulk-send.cc:1: /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
Note, this is a very simple example. It is also not strictly UDP, so it isn't in the right directory. I put it here as a quick test of this application. We found out that UDP flows are not working correctly with this application, likely due to this issue:
http://mailman.isi.edu/pipermail/ns-developers/2010-March/007607.html
Message from tomh.org@gmail.com
2010-10-13T04:47:48+00:00Tom Hendersonurn:md5:7f1ff891f4cd5ae9edbb78a082551bfb
My high-level comments are that:
1) this is a needed app, would like to merge for ns-3.10 if finished
2) I am skeptical that it will work predictably for UdpSocket since there is no backpressure (an issue discussed previously on the list but I'm not sure anyone ever fixed it)
3) no tests-- do you intend to add?
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.cc
File src/applications/bulk-send/bulk-send-application.cc (right):
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.cc#newcode128
src/applications/bulk-send/bulk-send-application.cc:128: void BulkSendApplication::StopApplication() // Called at time specified by Stop
suggest to run check-style.py on your new files; e.g. check-style.py -f bulk-send-application.cc -l 3 -i
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.cc#newcode176
src/applications/bulk-send/bulk-send-application.cc:176: NS_LOG_FUNCTION_NOARGS ();
NS_LOG_FUNCTION_NOARGS() is typically reserved for static functions; use NS_LOG_FUNCTION (this << socket); here instead.
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.cc#newcode188
src/applications/bulk-send/bulk-send-application.cc:188: void BulkSendApplication::DataSend(Ptr<Socket>, uint32_t)
why no logging in this function?
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.h
File src/applications/bulk-send/bulk-send-application.h (right):
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.h#newcode62
src/applications/bulk-send/bulk-send-application.h:62: * limit.
Clarify the interaction with StopApplication(). If StopApplication is called, then StartApplication(), will application start sending again (up to maxbytes)? Suggest to clarify also that this is an upper bound on the total number of bytes to send (if StopApplication is called, then possibly fewer than maxbytes will be sent). Also, I would say "no further application bytes is sent" rather than "no packet is sent again" since application can't control when a TCP segment is sent.
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.h#newcode65
src/applications/bulk-send/bulk-send-application.h:65:
It would be very helpful to provide a public: Ptr<Socket> GetSocket (void) const; because user may want to manipulate the socket (e.g. set attributes on it).
http://codereview.appspot.com/2408042/diff/1/src/helper/bulk-send-helper.h
File src/helper/bulk-send-helper.h (right):
http://codereview.appspot.com/2408042/diff/1/src/helper/bulk-send-helper.h#newcode57
src/helper/bulk-send-helper.h:57: * Helper function used to set the underlying application attributes.
clarify here that it is the BulkSendApplication attribute (e;g by saying "not the socket attribute"). Note that there is no easy way with this helper to set underlying socket attributes. (I guess one could downcast the Ptr<Application> obtained from the ApplicationContainer, and then use the GetSocket() method if implemented (see my other comment on the application itself)).
Message from unknown
2010-10-13T17:06:03+00:00Josh Pelkeyurn:md5:a2d9d3b1e6cef94a7de21202f803d039
Message from joshpelkey@gmail.com
2010-10-13T17:10:41+00:00Josh Pelkeyurn:md5:1c798cdeeb67ca359089bf21362b8863
1) We'd like to get this in as well
2) This is a confirmed issue using the udp example provided. We will have to look in to this more.
3) I think this is probably a good idea and will work on it once 2 is resolved.
Other updates below:
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.cc
File src/applications/bulk-send/bulk-send-application.cc (right):
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.cc#newcode128
src/applications/bulk-send/bulk-send-application.cc:128: void BulkSendApplication::StopApplication() // Called at time specified by Stop
On 2010/10/13 04:47:48, Tom Henderson wrote:
> suggest to run check-style.py on your new files; e.g. check-style.py -f
> bulk-send-application.cc -l 3 -i
Done.
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.cc#newcode176
src/applications/bulk-send/bulk-send-application.cc:176: NS_LOG_FUNCTION_NOARGS ();
On 2010/10/13 04:47:48, Tom Henderson wrote:
> NS_LOG_FUNCTION_NOARGS() is typically reserved for static functions; use
> NS_LOG_FUNCTION (this << socket); here instead.
Done.
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.cc#newcode188
src/applications/bulk-send/bulk-send-application.cc:188: void BulkSendApplication::DataSend(Ptr<Socket>, uint32_t)
On 2010/10/13 04:47:48, Tom Henderson wrote:
> why no logging in this function?
Done.
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.h
File src/applications/bulk-send/bulk-send-application.h (right):
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.h#newcode62
src/applications/bulk-send/bulk-send-application.h:62: * limit.
On 2010/10/13 04:47:48, Tom Henderson wrote:
> Clarify the interaction with StopApplication(). If StopApplication is called,
> then StartApplication(), will application start sending again (up to maxbytes)?
> Suggest to clarify also that this is an upper bound on the total number of bytes
> to send (if StopApplication is called, then possibly fewer than maxbytes will be
> sent). Also, I would say "no further application bytes is sent" rather than "no
> packet is sent again" since application can't control when a TCP segment is
> sent.
Done.
http://codereview.appspot.com/2408042/diff/1/src/applications/bulk-send/bulk-send-application.h#newcode65
src/applications/bulk-send/bulk-send-application.h:65:
On 2010/10/13 04:47:48, Tom Henderson wrote:
> It would be very helpful to provide a public: Ptr<Socket> GetSocket (void)
> const; because user may want to manipulate the socket (e.g. set attributes on
> it).
Done.
http://codereview.appspot.com/2408042/diff/1/src/helper/bulk-send-helper.h
File src/helper/bulk-send-helper.h (right):
http://codereview.appspot.com/2408042/diff/1/src/helper/bulk-send-helper.h#newcode57
src/helper/bulk-send-helper.h:57: * Helper function used to set the underlying application attributes.
On 2010/10/13 04:47:48, Tom Henderson wrote:
> clarify here that it is the BulkSendApplication attribute (e;g by saying "not
> the socket attribute"). Note that there is no easy way with this helper to set
> underlying socket attributes. (I guess one could downcast the Ptr<Application>
> obtained from the ApplicationContainer, and then use the GetSocket() method if
> implemented (see my other comment on the application itself)).
Done.
Message from tomh.org@gmail.com
2010-10-25T04:02:34+00:00Tom Hendersonurn:md5:800b38705aba905fd52a9474ef6574e5
I'd like to suggest a way forward for merging this application that does not involve blocking on finding a solution for UDP flow control (bug 1006).
1) Implement Socket::GetSockType() (analogous to getsockopt(SO_TYPE)) and have all ns-3 sockets support this.
2) Only allow sockets of type STREAM and SEQPACKET, and disallow BulkSendApplication to use SOCK_DGRAM or SOCK_RAW).
3) port ns-2 Agent/CBR (class CBR_Traffic) to ns-3 and only allow it to use SOCK_DGRAM or SOCK_RAW
---
1) and 2) would be prerequisites for merge of this app to ns-3; 3) would be nice to add at a later date
Message from joshpelkey@gmail.com
2010-12-06T21:10:25+00:00Josh Pelkeyurn:md5:746922e22363fd643962f24a95a29d37
Tom,
I implemented 1) here: http://codereview.appspot.com/3484041/
I plan to use this implementation and update BulkSend tomorrow.
--
Josh
On 2010/10/25 04:02:34, Tom Henderson wrote:
> I'd like to suggest a way forward for merging this application that does not
> involve blocking on finding a solution for UDP flow control (bug 1006).
>
> 1) Implement Socket::GetSockType() (analogous to getsockopt(SO_TYPE)) and have
> all ns-3 sockets support this.
>
> 2) Only allow sockets of type STREAM and SEQPACKET, and disallow
> BulkSendApplication to use SOCK_DGRAM or SOCK_RAW).
>
> 3) port ns-2 Agent/CBR (class CBR_Traffic) to ns-3 and only allow it to use
> SOCK_DGRAM or SOCK_RAW
>
> ---
>
> 1) and 2) would be prerequisites for merge of this app to ns-3; 3) would be nice
> to add at a later date
Message from unknown
2010-12-07T21:43:28+00:00Josh Pelkeyurn:md5:35837027d58e45bc8cdc510e9ccf56ea
Message from joshpelkey@gmail.com
2010-12-07T21:45:49+00:00Josh Pelkeyurn:md5:dd8a602b48d8e0a5e2ebdcb9706bf709
Patch set 3 requires the SO_TYPE patch here: http://codereview.appspot.com/3484041/
Now, when a user attempts to use BulkSend with anything other than a SOCK_STREAM or SOCK_SEQPACKET, NS_FATAL_ERROR is raised with the following message:
msg="Using BulkSend with an incompatible socket type. BulkSend requires SOCK_STREAM or SOCK_SEQPACKET. In other words, use TCP instead of UDP.", file=../src/applications/bulk-send/bulk-send-application.cc, line=126
--
Josh
terminate called without an active exception
On 2010/12/06 21:10:25, Josh Pelkey wrote:
> Tom,
>
> I implemented 1) here: http://codereview.appspot.com/3484041/
> I plan to use this implementation and update BulkSend tomorrow.
>
> --
> Josh
>
> On 2010/10/25 04:02:34, Tom Henderson wrote:
> > I'd like to suggest a way forward for merging this application that does not
> > involve blocking on finding a solution for UDP flow control (bug 1006).
> >
> > 1) Implement Socket::GetSockType() (analogous to getsockopt(SO_TYPE)) and have
> > all ns-3 sockets support this.
> >
> > 2) Only allow sockets of type STREAM and SEQPACKET, and disallow
> > BulkSendApplication to use SOCK_DGRAM or SOCK_RAW).
> >
> > 3) port ns-2 Agent/CBR (class CBR_Traffic) to ns-3 and only allow it to use
> > SOCK_DGRAM or SOCK_RAW
> >
> > ---
> >
> > 1) and 2) would be prerequisites for merge of this app to ns-3; 3) would be
> nice
> > to add at a later date
Message from joshpelkey@gmail.com
2010-12-07T21:47:08+00:00Josh Pelkeyurn:md5:adefd079333c311b6c0128f3ceb4b2a1
I forgot to update the release notes with these implementation details, but I will do that before merge.
--
Josh
On 2010/12/07 21:45:49, Josh Pelkey wrote:
> Patch set 3 requires the SO_TYPE patch here:
> http://codereview.appspot.com/3484041/
>
> Now, when a user attempts to use BulkSend with anything other than a SOCK_STREAM
> or SOCK_SEQPACKET, NS_FATAL_ERROR is raised with the following message:
>
> msg="Using BulkSend with an incompatible socket type. BulkSend requires
> SOCK_STREAM or SOCK_SEQPACKET. In other words, use TCP instead of UDP.",
> file=../src/applications/bulk-send/bulk-send-application.cc, line=126
>
> --
> Josh
>
> terminate called without an active exception
> On 2010/12/06 21:10:25, Josh Pelkey wrote:
> > Tom,
> >
> > I implemented 1) here: http://codereview.appspot.com/3484041/
> > I plan to use this implementation and update BulkSend tomorrow.
> >
> > --
> > Josh
> >
> > On 2010/10/25 04:02:34, Tom Henderson wrote:
> > > I'd like to suggest a way forward for merging this application that does not
> > > involve blocking on finding a solution for UDP flow control (bug 1006).
> > >
> > > 1) Implement Socket::GetSockType() (analogous to getsockopt(SO_TYPE)) and
> have
> > > all ns-3 sockets support this.
> > >
> > > 2) Only allow sockets of type STREAM and SEQPACKET, and disallow
> > > BulkSendApplication to use SOCK_DGRAM or SOCK_RAW).
> > >
> > > 3) port ns-2 Agent/CBR (class CBR_Traffic) to ns-3 and only allow it to use
> > > SOCK_DGRAM or SOCK_RAW
> > >
> > > ---
> > >
> > > 1) and 2) would be prerequisites for merge of this app to ns-3; 3) would be
> > nice
> > > to add at a later date
Message from tomh.org@gmail.com
2010-12-08T23:32:15+00:00Tom Hendersonurn:md5:334eace34456c96fbcb4c242a1e1bd69
A few last comments; I think this is ready to merge otherwise.
http://codereview.appspot.com/2408042/diff/18001/examples/tcp/tcp-bulk-send.cc
File examples/tcp/tcp-bulk-send.cc (right):
http://codereview.appspot.com/2408042/diff/18001/examples/tcp/tcp-bulk-send.cc#newcode24
examples/tcp/tcp-bulk-send.cc:24: // - Tracing of queues and packet receptions to file "tcp-bulk-send.tr"
and mention that pcap traces are being generated.
http://codereview.appspot.com/2408042/diff/18001/src/applications/bulk-send/bulk-send-application.cc
File src/applications/bulk-send/bulk-send-application.cc (right):
http://codereview.appspot.com/2408042/diff/18001/src/applications/bulk-send/bulk-send-application.cc#newcode63
src/applications/bulk-send/bulk-send-application.cc:63: TypeIdValue (UdpSocketFactory::GetTypeId ()),
should this default be TcpSocketFactory?
Message from unknown
2010-12-09T16:40:55+00:00Josh Pelkeyurn:md5:842a162cd24321747bf9cb348bd2c87c
Message from joshpelkey@gmail.com
2010-12-09T16:42:17+00:00Josh Pelkeyurn:md5:ef4dd71dcd7cff81b3bc79c25995e976
Documentation updated and change made for the TcpSocketFactory.
http://codereview.appspot.com/2408042/diff/18001/examples/tcp/tcp-bulk-send.cc
File examples/tcp/tcp-bulk-send.cc (right):
http://codereview.appspot.com/2408042/diff/18001/examples/tcp/tcp-bulk-send.cc#newcode24
examples/tcp/tcp-bulk-send.cc:24: // - Tracing of queues and packet receptions to file "tcp-bulk-send.tr"
On 2010/12/08 23:32:15, Tom Henderson wrote:
> and mention that pcap traces are being generated.
Done.
http://codereview.appspot.com/2408042/diff/18001/src/applications/bulk-send/bulk-send-application.cc
File src/applications/bulk-send/bulk-send-application.cc (right):
http://codereview.appspot.com/2408042/diff/18001/src/applications/bulk-send/bulk-send-application.cc#newcode63
src/applications/bulk-send/bulk-send-application.cc:63: TypeIdValue (UdpSocketFactory::GetTypeId ()),
On 2010/12/08 23:32:15, Tom Henderson wrote:
> should this default be TcpSocketFactory?
whoops, yes