post mid-term review

post mid-term review

[Tom Henderson, 2014-07-07 21:36:47]

There is a lot of good code here-- need to work towards getting it to a
mergeable state.  

I had specific questions about:
1) the need for a new application, even for testing
2) the need to touch the Socket base class
3) how MLDv2 will be enabled by the InternetStackHelper

https://codereview.appspot.com/111920044/diff/1/examples/ipv6/Mldv2-example.cc
File examples/ipv6/Mldv2-example.cc (right):

https://codereview.appspot.com/111920044/diff/1/examples/ipv6/Mldv2-example.c...
examples/ipv6/Mldv2-example.cc:30: // - Tracing of queues and packet receptions
to file "sample.tr"
Please update the above; it is copied from elsewhere and not accurate (sample.tr
is not being generated)

https://codereview.appspot.com/111920044/diff/1/examples/ipv6/Mldv2-example.c...
examples/ipv6/Mldv2-example.cc:39: NS_LOG_COMPONENT_DEFINE ("SampleExample");
change "SampleExample" to "Mldv2Example"

https://codereview.appspot.com/111920044/diff/1/examples/ipv6/Mldv2-example.c...
examples/ipv6/Mldv2-example.cc:90: SampleHelper sample;
I left a comment elsewhere that I do not see the need for Sample.  Why not use a
UdpEcho application?

I see in the wiki that this is intended for testing, but is there a problem with
what we already have?

https://codereview.appspot.com/111920044/diff/1/src/applications/model/sample.h
File src/applications/model/sample.h (right):

https://codereview.appspot.com/111920044/diff/1/src/applications/model/sample...
src/applications/model/sample.h:45: class Sample : public Application
I do not know why this is being introduced; we already have applications that
send IP packets.

https://codereview.appspot.com/111920044/diff/1/src/internet/helper/internet-...
File src/internet/helper/internet-stack-helper.cc (right):

https://codereview.appspot.com/111920044/diff/1/src/internet/helper/internet-...
src/internet/helper/internet-stack-helper.cc:242: m_mldv2Enabled (false)
why does this default to false?

https://codereview.appspot.com/111920044/diff/1/src/internet/helper/internet-...
File src/internet/helper/internet-stack-helper.h (right):

https://codereview.appspot.com/111920044/diff/1/src/internet/helper/internet-...
src/internet/helper/internet-stack-helper.h:206: void SetMldv2 (bool enable);
I would expect to see some logic more like this:

- if IPv6 is enabled and I have IPv6 router interfaces, this helper enables
Mldv2 (router behavior) by default

- if IPv6 is enabled and I later become a listener, this helper enables Mldv2
(listener behavior) by default

- if IPv6 is not enabled, obviously Mldv2 is not enabled.

I would expect also that hosts (non-routers) do not emit Mldv2 packets unless
they receive a query from a router, or they have an application who opens a
multicast socket or does an explicit join.  Is this the behavior?

Now, does anything new need to be explicitly enabled at this helper API?  How
does "router behavior" get enabled-- can a node learn this indirectly, or does
this helper need to expose a "Set router behavior" (either on a per-interface
basis, or for a whole node) of which MLDv2 router part is one behavior enabled?

https://codereview.appspot.com/111920044/diff/1/src/internet/model/ipv6-raw-s...
File src/internet/model/ipv6-raw-socket-impl.h (right):

https://codereview.appspot.com/111920044/diff/1/src/internet/model/ipv6-raw-s...
src/internet/model/ipv6-raw-socket-impl.h:140: void Ipv6JoinGroup (Ipv6Address
address,uint8_t socketMode,std::vector<Ipv6Address> sourceAddresses);
why not develop this in parallel to UdpSocket::MulticastJoinGroup()?

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.cc
File src/internet/model/mldv2.cc (right):

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.cc#n...
src/internet/model/mldv2.cc:34: using namespace std;
no 'using namespace std' in model code

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.cc#n...
src/internet/model/mldv2.cc:44: UintegerValue (30),
should this be a Time value instead of an integer?

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.cc#n...
src/internet/model/mldv2.cc:47: .AddAttribute ("DefaultResponseCode", "Maximum
time wait for reply",
Call this "MaximumResponseCode" to align with the standard.  No need to specify
"Default" in the naming of any of these attributes-- if you change the value
from the default, it is no longer a default.

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.cc#n...
src/internet/model/mldv2.cc:75: packet->RemoveHeader (query);
can you sanity check the return value here?  did the RemoveHeader succeed?

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.cc#n...
src/internet/model/mldv2.cc:110: ------------   --------------- 
----------------   -------
these will make good test cases for future tests.

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.cc#n...
src/internet/model/mldv2.cc:159: packet->RemoveHeader (report);
same comment-- check that RemoveHeader() succeeded

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h
File src/internet/model/mldv2.h (right):

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:3: * Copyright (c) 2007-2009 Strasbourg University
need to either add to or replace this copyright-- this is a new file.  If it was
copied and edited from somewhere else, preserve existing copyright, but if it is
almost entirely rewritten from the original file, just replace the copyright.

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:29: using namespace std;
delete the above; we do not use "using namespace std" within ns-3 models

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:32: * This are request which sockets made for a
perticular Multicast Address.
fix grammar and spelling of the above sentence.

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:38: uint8_t socketMode; //!< Mode of the socket.
why not an enum?

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:43: * This struct is used for tracing the state
changes on a given interface.
it is not used for tracing, but for tracking

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:47: Ipv6Address interfaceAddress; //!< Address of the
interface
most IPv6 interfaces have more than one address.  Why just a single address
here?

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:56: * \brief Mldv2 protoco implementation.
protocol

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:58: * This class behaves differently on a node and
router as Mldv2 is a asymmetric protocol.
an asymmetric

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:73: class Mldv2 : public Object
did you consider whether to split the host and router behaviour to two separate
classes?

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:79: * \return UID
it is the TypeId, not the UID

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:95: * \param index Index of the interface.
what index?   please specify whether this is an ipv6 interface index or not.

If this SendGeneralQuery() will cause scheduling of subsequent ones, what
happens when user repeatedly calls this?  Will each call to this method cause a
new one to be scheduled?

How does one stop the periodic sending of General Query?

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:105: void HandleListenerQuery (Ptr<Packet> packet,
Ipv6Address const &src, Ipv6Address const &dst, Ptr<Ipv6Interface> interface);
the explanation is too brief-- why would user call this public method?  Is this
a private method that is being exposed?

I think the answer is that other IPv6 protocol objects like ICMP need to be able
to access this-- if so, please clarify this.

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:110: * \param dst Destionation address.
Destination

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:113: void HandleListenerReport (Ptr<Packet> packet,
Ipv6Address const &src, Ipv6Address const &dst, Ptr<Ipv6Interface> interface);
same comment-- why would a user of MLDv2 protocol need to call
"HandleListenerReport()"  should this be private?  Explain in the doxygen, if
not.

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:122: class Entry : public Object
this seems to be getting complicated for the public API-- what is all of the
below doing?  Some unit testing and documentation around this class is needed,
IMO.

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:175: * \param query Icmpv6 mdv2 query header of the
packet.
MLDv2

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:181: * \param query Icmpv6 mdv2 query header of the
packet.
MLDv2

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:187: * \param query Icmpv6 mdv2 query header of the
packet.
MLDv2

https://codereview.appspot.com/111920044/diff/1/src/internet/model/mldv2.h#ne...
src/internet/model/mldv2.h:194: * \param address address of the multicast
address
simplify to
\param address multicast address

https://codereview.appspot.com/111920044/diff/1/src/network/model/socket.h
File src/network/model/socket.h (right):

https://codereview.appspot.com/111920044/diff/1/src/network/model/socket.h#ne...
src/network/model/socket.h:827: virtual void Ipv6JoinGroup (Ipv6Address
address,uint8_t socketMode,std::vector<Ipv6Address> sourceAddresses);
why is this needed?  We already have UdpSocket::MulticastJoinGroup() which
should be able to handle IPv6.

what is the socketMode and why is this not an enum?

suggest to delete this.