Added IPv6 extensions

[moatamri, 2009-10-30 11:31:30]

Review of IPv6 extensions

http://codereview.appspot.com/144048/diff/1/2
File examples/ipv6/fragmentation-ipv6.cc (right):

http://codereview.appspot.com/144048/diff/1/2#newcode66
Line 66: inline void PrintRoutingTable (Ptr<Node>& n)
why printing routines are inlined? Do we need to speed up printing?

http://codereview.appspot.com/144048/diff/1/2#newcode102
Line 102: CommandLine cmd;
what is this indentation, please use only the ns-3 coding style indentation.

http://codereview.appspot.com/144048/diff/1/2#newcode124
Line 124: NetDeviceContainer d1 = csma.Install (net1);
again weird indentation...

http://codereview.appspot.com/144048/diff/1/2#newcode141
Line 141: Time interPacketInterval = Seconds (1.);
you mean 1.0?

http://codereview.appspot.com/144048/diff/1/4
File examples/ipv6/loose-routing-ipv6.cc (right):

http://codereview.appspot.com/144048/diff/1/4#newcode65
Line 65: NS_LOG_INFO ("Create nodes.");
indentation

http://codereview.appspot.com/144048/diff/1/4#newcode96
Line 96: NetDeviceContainer d1 = csma.Install (net1);
why don't we use arrays here, it will reduce really a lot the code.

http://codereview.appspot.com/144048/diff/1/6
File src/applications/ping6/ping6.cc (right):

http://codereview.appspot.com/144048/diff/1/6#newcode202
Line 202: /* use routing type 0 */
what is type 0 routing? It is better to use static names for those kind of
variables so that we can understand what type of routing we are talking about.
It will be in this case LOOSE_ROUTING

http://codereview.appspot.com/144048/diff/1/14
File src/internet-stack/ipv6-extension-demux.h (right):

http://codereview.appspot.com/144048/diff/1/14#newcode31
Line 31: class Ipv6Extension;
some more doxygen please?

http://codereview.appspot.com/144048/diff/1/14#newcode39
Line 39: {
coding style

http://codereview.appspot.com/144048/diff/1/15
File src/internet-stack/ipv6-extension-header.cc (right):

http://codereview.appspot.com/144048/diff/1/15#newcode530
Line 530: /* TODO */
why they are left here if they are not done yet? there are some plans to finish
the TODOs?

http://codereview.appspot.com/144048/diff/1/16
File src/internet-stack/ipv6-extension-header.h (right):

http://codereview.appspot.com/144048/diff/1/16#newcode36
Line 36: */
Coding style:indenting...

http://codereview.appspot.com/144048/diff/1/17
File src/internet-stack/ipv6-extension.cc (right):

http://codereview.appspot.com/144048/diff/1/17#newcode87
Line 87: const uint8_t Ipv6ExtensionHopByHop::EXT_NUMBER = 0;
should be defined in the header

http://codereview.appspot.com/144048/diff/1/17#newcode150
Line 150: if (ipv6Option == 0)
some comments about the options type?

http://codereview.appspot.com/144048/diff/1/17#newcode169
Line 169: /* icmpv6->SendErrorParameterError (malformedPacket, dst,
ipv6Header.GetSourceAddress (), Icmpv6Header::ICMPV6_UNKNOWN_OPTION, offset +
processedSize); */
either remove the commented code or enable it, same for the following code

http://codereview.appspot.com/144048/diff/1/17#newcode190
Line 190: break;
should also drop or generate an error this is also an anormal option right?

http://codereview.appspot.com/144048/diff/1/17#newcode208
Line 208: const uint8_t Ipv6ExtensionDestination::EXT_NUMBER = 60;
in header

http://codereview.appspot.com/144048/diff/1/17#newcode236
Line 236: uint8_t Ipv6ExtensionDestination::Process (Ptr<Packet>& packet,
uint8_t offset, Ipv6Header const& ipv6Header, Ipv6Address dst, uint8_t
*nextHeader, bool& isDropped)
Looks like copy paste function with minor changes, can you implement this in a
less redundant way?

http://codereview.appspot.com/144048/diff/1/17#newcode271
Line 271: if (ipv6Option == 0)
same comments as the above similar function

http://codereview.appspot.com/144048/diff/1/17#newcode425
Line 425: return 0;
function doesn't return any other value, this is useless I think.

http://codereview.appspot.com/144048/diff/1/17#newcode428
Line 428: void Ipv6ExtensionFragment::GetFragments (Ptr<Packet> packet, uint32_t
maxFragmentSize, std::list<Ptr<Packet> >& listFragments)
I would recommand to divide this big function into multiple functions or to
isolate some blocks into private routines, this will make the function much more
readable.

http://codereview.appspot.com/144048/diff/1/17#newcode495
Line 495: Ipv6ExtensionDestinationHeader *destinationHeader = new
Ipv6ExtensionDestinationHeader ();
Please verify that there is a delete somewhere for each new statement.

http://codereview.appspot.com/144048/diff/1/17#newcode664
Line 664: /* XXX : FIX THIS BUG (NS_ASSERT WHEN PRINT PACKET INFORMATION */
Either fix or remove commented code.

http://codereview.appspot.com/144048/diff/1/17#newcode904
Line 904: /* TODO */
same as above

http://codereview.appspot.com/144048/diff/1/17#newcode949
Line 949: /* short-circuiting routing stuff */
more explanations?

http://codereview.appspot.com/144048/diff/1/17#newcode951
Line 951: /* XXX taken from src/helper/ipv6-static-rouging.cc
ipv6-static-routing.cc

http://codereview.appspot.com/144048/diff/1/17#newcode1034
Line 1034: uint8_t Ipv6ExtensionESP::Process (Ptr<Packet>& packet, uint8_t
offset, Ipv6Header const& ipv6Header, Ipv6Address dst, uint8_t *nextHeader,
bool& isDropped)
planning to do it shortly?

http://codereview.appspot.com/144048/diff/1/18
File src/internet-stack/ipv6-extension.h (right):

http://codereview.appspot.com/144048/diff/1/18#newcode49
Line 49: public:
indenting in all you header files

http://codereview.appspot.com/144048/diff/1/19
File src/internet-stack/ipv6-l3-protocol.cc (right):

http://codereview.appspot.com/144048/diff/1/19#newcode784
Line 784: /* NS_ASSERT (packet->GetSize () <= outInterface->GetDevice ()->GetMtu
()); */
remove commented code

http://codereview.appspot.com/144048/diff/1/19#newcode934
Line 934: /*GetProtocol (ip.GetNextHeader ());*/
remove commented code and please add some comments to the additions you did to
this file

http://codereview.appspot.com/144048/diff/1/19#newcode976
Line 976: /* GetIcmpv6 ()->SendErrorParameterError (malformedPacket, dst, src,
Icmpv6Header::ICMPV6_UNKNOWN_NEXT_HEADER, 40); */
either uncomment or delete

http://codereview.appspot.com/144048/diff/1/19#newcode1015
Line 1015: #if 0
this code will be useful in the future?

http://codereview.appspot.com/144048/diff/1/25
File src/internet-stack/ipv6-option.cc (right):

http://codereview.appspot.com/144048/diff/1/25#newcode152
Line 152: const uint8_t Ipv6OptionJumbogram::OPT_NUMBER = 44;
why not in header file?

http://codereview.appspot.com/144048/diff/1/29
File src/routing/static-routing/ipv6-static-routing.cc (right):

http://codereview.appspot.com/144048/diff/1/29#newcode532
Line 532: header.GetDestinationAddress (),
remove white space changes

http://codereview.appspot.com/144048/diff/1/30
File src/routing/static-routing/ipv6-static-routing.h (right):

http://codereview.appspot.com/144048/diff/1/30#newcode239
Line 239: 
why make this method public instead of private, any reason?

[ping67, 2009-11-02 17:09:10]

Repository http://svnet.u-strasbg.fr/hg/ns-3-ipv6-ext/ has been updated.

http://codereview.appspot.com/144048/diff/1/2
File examples/ipv6/fragmentation-ipv6.cc (right):

http://codereview.appspot.com/144048/diff/1/2#newcode66
Line 66: inline void PrintRoutingTable (Ptr<Node>& n)
On 2009/10/30 11:31:30, moatamri wrote:
> why printing routines are inlined? Do we need to speed up printing?

Methods defined in class body are by default inline so I just make it explicit.

http://codereview.appspot.com/144048/diff/1/2#newcode102
Line 102: CommandLine cmd;
Done

http://codereview.appspot.com/144048/diff/1/2#newcode124
Line 124: NetDeviceContainer d1 = csma.Install (net1);
On 2009/10/30 11:31:30, moatamri wrote:
> again weird indentation...

Done.

http://codereview.appspot.com/144048/diff/1/2#newcode141
Line 141: Time interPacketInterval = Seconds (1.);
On 2009/10/30 11:31:30, moatamri wrote:
> you mean 1.0?

Done.

http://codereview.appspot.com/144048/diff/1/4
File examples/ipv6/loose-routing-ipv6.cc (right):

http://codereview.appspot.com/144048/diff/1/4#newcode65
Line 65: NS_LOG_INFO ("Create nodes.");
On 2009/10/30 11:31:30, moatamri wrote:
> indentation

Done.

http://codereview.appspot.com/144048/diff/1/6
File src/applications/ping6/ping6.cc (right):

http://codereview.appspot.com/144048/diff/1/6#newcode202
Line 202: /* use routing type 0 */
On 2009/10/30 11:31:30, moatamri wrote:
> what is type 0 routing? It is better to use static names for those kind of
> variables so that we can understand what type of routing we are talking about.
> It will be in this case LOOSE_ROUTING

Done.

http://codereview.appspot.com/144048/diff/1/14
File src/internet-stack/ipv6-extension-demux.h (right):

http://codereview.appspot.com/144048/diff/1/14#newcode39
Line 39: {
On 2009/10/30 11:31:30, moatamri wrote:
> coding style

Done.

http://codereview.appspot.com/144048/diff/1/15
File src/internet-stack/ipv6-extension-header.cc (right):

http://codereview.appspot.com/144048/diff/1/15#newcode530
Line 530: /* TODO */
We (Strasbourg University) have no plans to address it soon, but we just left it
for (future) contributors who would like to work on it.

http://codereview.appspot.com/144048/diff/1/16
File src/internet-stack/ipv6-extension-header.h (right):

http://codereview.appspot.com/144048/diff/1/16#newcode36
Line 36: */
On 2009/10/30 11:31:30, moatamri wrote:
> Coding style:indenting...

Done.

http://codereview.appspot.com/144048/diff/1/17
File src/internet-stack/ipv6-extension.cc (right):

http://codereview.appspot.com/144048/diff/1/17#newcode87
Line 87: const uint8_t Ipv6ExtensionHopByHop::EXT_NUMBER = 0;
Done.

http://codereview.appspot.com/144048/diff/1/17#newcode169
Line 169: /* icmpv6->SendErrorParameterError (malformedPacket, dst,
ipv6Header.GetSourceAddress (), Icmpv6Header::ICMPV6_UNKNOWN_OPTION, offset +
processedSize); */
On 2009/10/30 11:31:30, moatamri wrote:
> either remove the commented code or enable it, same for the following code

Done.

http://codereview.appspot.com/144048/diff/1/17#newcode190
Line 190: break;
On 2009/10/30 11:31:30, moatamri wrote:
> should also drop or generate an error this is also an anormal option right?

Done.

http://codereview.appspot.com/144048/diff/1/17#newcode208
Line 208: const uint8_t Ipv6ExtensionDestination::EXT_NUMBER = 60;
On 2009/10/30 11:31:30, moatamri wrote:
> in header

Done.

http://codereview.appspot.com/144048/diff/1/17#newcode236
Line 236: uint8_t Ipv6ExtensionDestination::Process (Ptr<Packet>& packet,
uint8_t offset, Ipv6Header const& ipv6Header, Ipv6Address dst, uint8_t
*nextHeader, bool& isDropped)
I will think of this at the end of week or next week.

http://codereview.appspot.com/144048/diff/1/17#newcode425
Line 425: return 0;
It is implementation of virtual function of superclass so cannot change return
or not value.

http://codereview.appspot.com/144048/diff/1/17#newcode428
Line 428: void Ipv6ExtensionFragment::GetFragments (Ptr<Packet> packet, uint32_t
maxFragmentSize, std::list<Ptr<Packet> >& listFragments)
Idem I will think of it end of week or next week.

http://codereview.appspot.com/144048/diff/1/17#newcode495
Line 495: Ipv6ExtensionDestinationHeader *destinationHeader = new
Ipv6ExtensionDestinationHeader ();
Deleted in line 576.

http://codereview.appspot.com/144048/diff/1/17#newcode664
Line 664: /* XXX : FIX THIS BUG (NS_ASSERT WHEN PRINT PACKET INFORMATION */
On 2009/10/30 11:31:30, moatamri wrote:
> Either fix or remove commented code.

Done.

http://codereview.appspot.com/144048/diff/1/17#newcode904
Line 904: /* TODO */
On 2009/10/30 11:31:30, moatamri wrote:
> same as above

Done.

http://codereview.appspot.com/144048/diff/1/17#newcode949
Line 949: /* short-circuiting routing stuff */
On 2009/10/30 11:31:30, moatamri wrote:
> more explanations?

Done.

http://codereview.appspot.com/144048/diff/1/17#newcode951
Line 951: /* XXX taken from src/helper/ipv6-static-rouging.cc
On 2009/10/30 11:31:30, moatamri wrote:
> ipv6-static-routing.cc

Done.

http://codereview.appspot.com/144048/diff/1/17#newcode1034
Line 1034: uint8_t Ipv6ExtensionESP::Process (Ptr<Packet>& packet, uint8_t
offset, Ipv6Header const& ipv6Header, Ipv6Address dst, uint8_t *nextHeader,
bool& isDropped)
Shortly ? no. At least not for ns-3.7

http://codereview.appspot.com/144048/diff/1/18
File src/internet-stack/ipv6-extension.h (right):

http://codereview.appspot.com/144048/diff/1/18#newcode49
Line 49: public:
On 2009/10/30 11:31:30, moatamri wrote:
> indenting in all you header files

Done.

http://codereview.appspot.com/144048/diff/1/19
File src/internet-stack/ipv6-l3-protocol.cc (right):

http://codereview.appspot.com/144048/diff/1/19#newcode784
Line 784: /* NS_ASSERT (packet->GetSize () <= outInterface->GetDevice ()->GetMtu
()); */
On 2009/10/30 11:31:30, moatamri wrote:
> remove commented code

Done.

http://codereview.appspot.com/144048/diff/1/19#newcode976
Line 976: /* GetIcmpv6 ()->SendErrorParameterError (malformedPacket, dst, src,
Icmpv6Header::ICMPV6_UNKNOWN_NEXT_HEADER, 40); */
On 2009/10/30 11:31:30, moatamri wrote:
> either uncomment or delete

Done.

http://codereview.appspot.com/144048/diff/1/19#newcode1015
Line 1015: #if 0
On 2009/10/30 11:31:30, moatamri wrote:
> this code will be useful in the future?

Done.

http://codereview.appspot.com/144048/diff/1/25
File src/internet-stack/ipv6-option.cc (right):

http://codereview.appspot.com/144048/diff/1/25#newcode152
Line 152: const uint8_t Ipv6OptionJumbogram::OPT_NUMBER = 44;
On 2009/10/30 11:31:30, moatamri wrote:
> why not in header file?

Done.

http://codereview.appspot.com/144048/diff/1/29
File src/routing/static-routing/ipv6-static-routing.cc (right):

http://codereview.appspot.com/144048/diff/1/29#newcode532
Line 532: header.GetDestinationAddress (),
On 2009/10/30 11:31:30, moatamri wrote:
> remove white space changes

Done.

http://codereview.appspot.com/144048/diff/1/30
File src/routing/static-routing/ipv6-static-routing.h (right):

http://codereview.appspot.com/144048/diff/1/30#newcode239
Line 239: 
We need this in Ipv6LooseRoutingExtension::Process to perform a lookup in
routing table. Possible alternatives are given in one of my mail dated on 26th
october ("Re: [Ns-developers] IPv6 extension support").

[ping67, 2009-11-11 15:31:33]

http://codereview.appspot.com/144048/diff/1/17
File src/internet-stack/ipv6-extension.cc (right):

http://codereview.appspot.com/144048/diff/1/17#newcode236
src/internet-stack/ipv6-extension.cc:236: uint8_t
Ipv6ExtensionDestination::Process (Ptr<Packet>& packet, uint8_t offset,
Ipv6Header const& ipv6Header, Ipv6Address dst, uint8_t *nextHeader, bool&
isDropped)
On 2009/11/02 17:09:10, ping67 wrote:
> I will think of this at the end of week or next week.
> 

Done.

http://codereview.appspot.com/144048/diff/1/17#newcode271
src/internet-stack/ipv6-extension.cc:271: if (ipv6Option == 0)
On 2009/10/30 11:31:30, moatamri wrote:
> same comments as the above similar function

Done.