Moved the internet stack creation to the helper, refactor some functions and files and repla

[Tom Henderson, 2009-05-18 05:46:47]

Faker, see my comments and questions inline.

http://codereview.appspot.com/63068/diff/1/2
File src/helper/internet-stack-helper.cc (right):

http://codereview.appspot.com/63068/diff/1/2#newcode237
Line 237: m_tcpFactory.Create<Object> ()->AggregateObject (node);
please add a comment here why all other aggregations are of the form
node->AggregateObject (protocol) but here we have (TCP) factory->AggregateObject
(node)

http://codereview.appspot.com/63068/diff/1/3
File src/helper/internet-stack-helper.h (right):

http://codereview.appspot.com/63068/diff/1/3#newcode77
Line 77: * to be setup, please use instead the following SetTcp implementation.
what happens if a user mistakenly sets NSC with this function?  Your reference
to "following" could be more specific by saying "the version that requires an
attribute" (or else do not overload SetTcp and instead call it
SetTcpWithAttribute()).  Also, s/functions/function/

http://codereview.appspot.com/63068/diff/1/3#newcode95
Line 95: void SetTcp (std::string tid, std::string n0, const AttributeValue
&v0);
would be clearer to rename "n0" to something like "attr" (and "v0" to something
like "val")

http://codereview.appspot.com/63068/diff/1/10
File src/internet-stack/ipv4-l3-protocol.cc (right):

http://codereview.appspot.com/63068/diff/1/10#newcode163
Line 163: }
based on the doxygen, I expected to see some logic here to handle notifications
that a new L4 protocol has been aggregated.

http://codereview.appspot.com/63068/diff/1/17
File src/internet-stack/tcp-test.cc (right):

http://codereview.appspot.com/63068/diff/1/17#newcode6
Line 6: */
please add gpl and emacs mode line, and clarify whether this is for all Tcp
socket tests or just TcpSocketImpl

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

http://codereview.appspot.com/63068/diff/1/18#newcode98
Line 98: }
It seems to me that this implementation relies on the aggregation being
performed in a specific order (Ipv4L3Protocol before UdpL4Protocol)-- is that
intended?  The doxygen comments for all of these NotifyNewAggregate () functions
do not seem to me to be strictly correct; the doxygen suggests that other
objects will be notified of the existence of this new object, but instead what
seems to be going on is the new object is not notifying anyone but is just doing
the plumbing in this function.  (perhaps I misunderstand what is intended)

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

http://codereview.appspot.com/63068/diff/1/21#newcode1
Line 1: #ifdef RUN_SELF_TESTS
GPL and emacs mode missing

[Tom Henderson, 2009-05-18 05:50:43]

On 2009/05/18 05:46:47, Tom H. wrote:
> Faker, see my comments and questions inline.
> 
> http://codereview.appspot.com/63068/diff/1/2
> File src/helper/internet-stack-helper.cc (right):
> 
> http://codereview.appspot.com/63068/diff/1/2#newcode237
> Line 237: m_tcpFactory.Create<Object> ()->AggregateObject (node);
> please add a comment here why all other aggregations are of the form
> node->AggregateObject (protocol) but here we have (TCP)
factory->AggregateObject
> (node)
> 
> http://codereview.appspot.com/63068/diff/1/3
> File src/helper/internet-stack-helper.h (right):
> 
> http://codereview.appspot.com/63068/diff/1/3#newcode77
> Line 77: * to be setup, please use instead the following SetTcp
implementation.
> what happens if a user mistakenly sets NSC with this function?  Your reference
> to "following" could be more specific by saying "the version that requires an
> attribute" (or else do not overload SetTcp and instead call it
> SetTcpWithAttribute()).  Also, s/functions/function/
> 
> http://codereview.appspot.com/63068/diff/1/3#newcode95
> Line 95: void SetTcp (std::string tid, std::string n0, const AttributeValue
> &v0);
> would be clearer to rename "n0" to something like "attr" (and "v0" to
something
> like "val")
> 
> http://codereview.appspot.com/63068/diff/1/10
> File src/internet-stack/ipv4-l3-protocol.cc (right):
> 
> http://codereview.appspot.com/63068/diff/1/10#newcode163
> Line 163: }
> based on the doxygen, I expected to see some logic here to handle
notifications
> that a new L4 protocol has been aggregated.
> 
> http://codereview.appspot.com/63068/diff/1/17
> File src/internet-stack/tcp-test.cc (right):
> 
> http://codereview.appspot.com/63068/diff/1/17#newcode6
> Line 6: */
> please add gpl and emacs mode line, and clarify whether this is for all Tcp
> socket tests or just TcpSocketImpl
> 
> http://codereview.appspot.com/63068/diff/1/18
> File src/internet-stack/udp-l4-protocol.cc (right):
> 
> http://codereview.appspot.com/63068/diff/1/18#newcode98
> Line 98: }
> It seems to me that this implementation relies on the aggregation being
> performed in a specific order (Ipv4L3Protocol before UdpL4Protocol)-- is that
> intended?  The doxygen comments for all of these NotifyNewAggregate ()
functions
> do not seem to me to be strictly correct; the doxygen suggests that other
> objects will be notified of the existence of this new object, but instead what
> seems to be going on is the new object is not notifying anyone but is just
doing
> the plumbing in this function.  (perhaps I misunderstand what is intended)
> 
> http://codereview.appspot.com/63068/diff/1/21
> File src/internet-stack/udp-test.cc (right):
> 
> http://codereview.appspot.com/63068/diff/1/21#newcode1
> Line 1: #ifdef RUN_SELF_TESTS
> GPL and emacs mode missing

[faker.moatamri, 2009-05-18 13:31:07]

http://codereview.appspot.com/63068/diff/1/2
File src/helper/internet-stack-helper.cc (right):

http://codereview.appspot.com/63068/diff/1/2#newcode237
Line 237: m_tcpFactory.Create<Object> ()->AggregateObject (node);
I will inverse it to call node->AggregateObject (protocol)

http://codereview.appspot.com/63068/diff/1/3
File src/helper/internet-stack-helper.h (right):

http://codereview.appspot.com/63068/diff/1/3#newcode95
Line 95: void SetTcp (std::string tid, std::string n0, const AttributeValue
&v0);
On 2009/05/18 05:46:47, Tom H. wrote:
> would be clearer to rename "n0" to something like "attr" (and "v0" to
something
> like "val")

Done.

http://codereview.appspot.com/63068/diff/1/10
File src/internet-stack/ipv4-l3-protocol.cc (right):

http://codereview.appspot.com/63068/diff/1/10#newcode163
Line 163: }
On 2009/05/18 05:46:47, Tom H. wrote:
> based on the doxygen, I expected to see some logic here to handle
notifications
> that a new L4 protocol has been aggregated.
It's the layer 4 that notifies layer3, please refer to 
NscTcpL4Protocol::NotifyNewAggregate. Line: ipv4->Insert (this).

http://codereview.appspot.com/63068/diff/1/17
File src/internet-stack/tcp-test.cc (right):

http://codereview.appspot.com/63068/diff/1/17#newcode6
Line 6: */
On 2009/05/18 05:46:47, Tom H. wrote:
> please add gpl and emacs mode line, and clarify whether this is for all Tcp
> socket tests or just TcpSocketImpl

Done.

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

http://codereview.appspot.com/63068/diff/1/18#newcode98
Line 98: }
Actually the calls are not in a specific order, we just need to verify that
everything is there to do the needed aggregation, I changed the code as
suggested by mathieu to reflect this idea.

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

http://codereview.appspot.com/63068/diff/1/21#newcode1
Line 1: #ifdef RUN_SELF_TESTS
On 2009/05/18 05:46:47, Tom H. wrote:
> GPL and emacs mode missing

Done.