[Ns-developers] Design Review of ns-3-ipv6-1st (ns-3.3 new feature)

[Mathieu Lacage]

hi all,


I apologize very much for this _very_ late review: it's all my fault,
etc. A couple of comments:


a) In src/node/ipv6-header.h, you certainly did not intend to create a
member variable named nextHeader of type "enum NextHeader_e"

class Ipv6Header : public Header
{
  [snip]
    enum NextHeader_e
    {
  [snip]
    }nextHeader;

b) In src/node/ipv6-header.cc, 

void Ipv6Header::Serialize(Buffer::Iterator start) const
{
  Buffer::Iterator i = start;
  uint8_t src[16];
  uint8_t dst[16];

  [snip]

  m_sourceAddress.Serialize(src);
  m_destinationAddress.Serialize(dst);
  i.Write(src, 16);
  i.Write(dst, 16);
}

It would be nice to match the ipv4 code and:
1) define and implement WriteTo and ReadFrom in
src/node/address-utils.h/cc
2) use them here instead of using a temporary buffer of raw 16 bytes 
3) The same goes for Deserialize

c) What is the purpose of an icmp socket ? In the normal socket API,
icmp messages are delivered to each protocol-specific sockets through
ancillary messages while icmp messages are sent over raw ip sockets

d) I see that you added GetMulticast6 and MakeMulticast6Address in
NetDevice to mirror GetMulticast and MakeMulticast. I have to confess
that I still don't understand the purpose of the unused GetMulticast/6
(see bug 294) so, it's hard to feel excited about this addition. i.e., I
understand the purpose of MakeMulticastAddress and I understand the
desire to mirror the GetBroadcast method but I really don't understand
why MakeMulticastAddress could not be renamed to GetMulticast and why we
could not get rid of the never-used current-GetMulticast method.
Basically, I support the merging of NetDevice::MakeMulticastAddress into
the method named NetDevice::GetMulticast or the removal of the
currently-unused GetMulticast method (which, I have to confess, I can't
imagine useful at all: is there a use-case where it could be useful in
its current form ?).

e) It would be helpful to move the code in
CsmaNetDevice::MakeMulticast6Address to Mac48Address::GetMulticast
(Ipv6Prefix); to mirror the ipv4 implementation and improve code reuse
across multiple device implementations.


On Thu, 2008-10-09 at 11:06 -0700, craigdo at ee.washington.edu wrote:
> Hi all,
> 
> We have received a request to merge a new feature into ns-3.3 (the upcoming
> stable release of ns-3 scheduled for release in December).  Based on our
> recently updated release process, we have to do a design review of the
> feature addition, and the current maintainers of the ns-3 codebase have to
> sign off on it, before we merge it into ns-3-dev and then into ns-3.3.
> 
> I sent the below email to the maintainers yesterday requesting an "official"
> design review for Sebastien Vincent's ns-3-ipv6-1st feature.  The
> maintainers are in general agreement that they want to do the review in a
> public setting and also solicit input from people on the ns-developers
> mailing list.  This means some technical emails will probably start flying
> around regarding possibly abstract questions of IPv6 support in ns-3.  I
> wanted to provide some context before that happens.
> 
> So, this email is an introduction to the design review thread for the first
> piece of the IPv6 support in ns-3.  As far as timing goes, this all needs to
> be wrapped up by about the end of the month.  Anyone reading should feel
> free to take a look at the submitted code and voice opinions.
> 
> Regards,
> 
> -- Craig
> 
> > You are receiving this email because you are a maintainer of 
> > ns-3 and a new
> > feature addition is being considered for ns-3.3.
> > 
> > We have a submission of "ns-3-ipv6-1st" to be considered as a 
> > new feature
> > addition for ns-3.3.  As indicated in our release process, 
> > this submission
> > crosses maintainer responsibilities and so a design review 
> > should be done.
> > Also this could be a fairly significant change in terms of 
> > usability and API
> > that could be very visible in the future.
> > 
> > Before proceeding I'll need some kind of positive 
> > acknowledgement from each
> > of you that you don't have any issues and that we should go 
> > ahead and merge
> > this patch.
> > 
> > If there are any discussions about the change, it would 
> > probably be good to
> > copy everyone.
> > 
> > The following is the email from Sebastien 
> > 
> > > -----Original Message-----
> > > From: Sébastien Vincent [mailto:vincent at clarinet.u-strasbg.fr] 
> > > Sent: Wednesday, October 08, 2008 2:30 AM
> > > To: craigdo at u.washington.edu
> > > Cc: 'Tom Henderson'; 'Mathieu Lacage'; 'Craig Dowell'
> > > Subject: ns-3-ipv6-1st 
> > > 
> > > Hi Craig,
> > > 
> > > Code :
> > > ====
> > > 
> > > The repository that contains the first chunk of IPv6 is located at 
> > > http://code.nsnam.org/vincent/ns-3-ipv6-1st/ and it is 
> > > synchronized with 
> > > ns-3-dev. The corresponding patch file is attached.
> > > 
> > > Summary of changes :
> > > ==============
> > > 
> > > The features of this first chunk of IPv6 provide the addresses, the 
> > > routes, the IPv6 header and some changes in *NetDevice to 
> > > allow specific 
> > > ethernet multicast addresses to be forward up to layer 3.
> > > 
> > > Files added : 
> > > - src/node/ipv6-route.cc/h;
> > > - src/node/ipv6-address.cc/h;
> > > - src/node/inet6-socket-address.cc,h;
> > > - src/node/ipv6-header.cc,h;
> > > - src/node/icmp-socket.cc,h;
> > > - example/test-ipv6.cc.
> > > 
> > > Changes on existing code :
> > > - Add virtual method GetMulticast6 and MakeMulticast6Address  in 
> > > NetDevice and all of its subclasses (CsmaNetDevice, ...);
> > > - In CsmaNetDevice, we declare some ethernet representation of 
> > > well-known IPv6 multicast address, and forward up all packet 
> > > (that match 
> > > these addresses) to layer 3.
> > > 
> > > This first chunk is not sufficient to do IPv6 simulations, 
> > it is just 
> > > the addresses and the changes in NetDevice. In very old 
> > > previous mail, 
> > > we planned to include the layer 3 (Ipv6L3Protocol, ...) in 
> > > the second chunk.
> > > 
> > > Maybe it will be good to have IPv6 layer 3 protocol, ICMPv6 / 
> > > Neighbor 
> > > Discovery protocol and a Ping6 application & example. What do 
> > > you think ?
> > > 
> > > Regards,
> > > --
> > > Sebastien Vincent
> > > Network and Research team -  University of Strasbourg
> > > 
> > > 
> > 
> 
> 
>