MPLS implementation

[Lalith Suresh, 2010-04-14 17:50:36]

The helper related code should go into src/helper/mpls-stack-helper.h/cc.

[Lalith Suresh, 2010-04-14 18:02:29]

On 2010/04/14 17:50:36, Lalith Suresh wrote:
> The helper related code should go into src/helper/mpls-stack-helper.h/cc.

Sorry if that looked confusing, I meant, src/helper/

[aachurin, 2010-04-18 07:06:00]

On 2010/04/14 18:02:29, Lalith Suresh wrote:
> On 2010/04/14 17:50:36, Lalith Suresh wrote:
> > The helper related code should go into src/helper/mpls-stack-helper.h/cc.
> 
> Sorry if that looked confusing, I meant, src/helper/

Yes, it will be in next version

[aachurin, 2010-04-18 07:12:35]

On 2010/04/18 07:06:00, aachurin wrote:
> On 2010/04/14 18:02:29, Lalith Suresh wrote:
> > On 2010/04/14 17:50:36, Lalith Suresh wrote:
> > > The helper related code should go into src/helper/mpls-stack-helper.h/cc.
> > 
> > Sorry if that looked confusing, I meant, src/helper/
> 
> Yes, it will be in next version

I have changed the class names:
ipv4-to-mpls-routing-protocol => ipv4-to-mpls-routing
ipv6-to-mpls-routing-protocol => ipv6-to-mpls-routing

[Mathieu Lacage, 2010-05-21 13:26:12]

I am short on time so I did not review completely the MplsRoutingProtocol class
but this is real nice code. I support merging it.

http://codereview.appspot.com/850045/diff/1/5
File src/mpls-stack/mpls/forwarding-equivalence-class.h (right):

http://codereview.appspot.com/850045/diff/1/5#newcode38
src/mpls-stack/mpls/forwarding-equivalence-class.h:38: virtual uint32_t GetMatch
(Ptr<const Packet> packet, const Ipv4Header &header) const;
could you document what the return value means ? (and what the default behavior
does ?)

http://codereview.appspot.com/850045/diff/1/11
File src/mpls-stack/mpls/ipv4-host-address.h (right):

http://codereview.appspot.com/850045/diff/1/11#newcode36
src/mpls-stack/mpls/ipv4-host-address.h:36: class Ipv4HostAddress : public
ForwardingEquivalenceClass
It looks like Ipv4HostAddress is a special case of Ipv4AddressPrefix with a
prefix size of 32, right ?

http://codereview.appspot.com/850045/diff/1/17
File src/mpls-stack/mpls/label-information-base-helper.h (right):

http://codereview.appspot.com/850045/diff/1/17#newcode47
src/mpls-stack/mpls/label-information-base-helper.h:47: uint32_t outLabel)
const;
What I would probably do here is that, instead of using a class with a single
data member to forward the calls to, I would instead do this:

namespace LabelInformationBaseHelper {

void InstallEntry (Ptr<Node> node, const ForwardingEquivalenceClass &fec, ...)

} 

The above would make it more obvious that there is no state in this piece of
code.

http://codereview.appspot.com/850045/diff/1/17#newcode49
src/mpls-stack/mpls/label-information-base-helper.h:49: /* just shortcut,
outIfIndex = -1 */
what are the semantics of outIfIndex = -1 ? Can you document it in the method
declaration above ?

http://codereview.appspot.com/850045/diff/1/17#newcode58
src/mpls-stack/mpls/label-information-base-helper.h:58: uint32_t outLabel = -1)
const;
others will disagree with this comment but I personally have been trying really
hard to use method with parameters which have default values. The main reason is
that if you use this together with overloaded methods (which you do here),
client code starts to become confusing because it becomes hard to tell which
method is called by the client code.

Another reason why mixing overloaded methods and parameters with default values
is a bad idea is that if someone has the stupid idea of using them with
templates (say, with MakeCallback), really really bad things will happen. This
is how kittens get killed and no one wants to be responsible for dead kittens.

http://codereview.appspot.com/850045/diff/1/19
File src/mpls-stack/mpls/mpls-label-stack.h (right):

http://codereview.appspot.com/850045/diff/1/19#newcode44
src/mpls-stack/mpls/mpls-label-stack.h:44: static const uint32_t
MPLS_LABEL_IPV4NULL = 0;
I personally would remove the MPLS_LABEL_ prefix because user code has to use
the MplsLabelStackEntry prefix which is un-ambiguous.

http://codereview.appspot.com/850045/diff/1/22
File src/mpls-stack/mpls/mpls-routing-protocol.cc (right):

http://codereview.appspot.com/850045/diff/1/22#newcode79
src/mpls-stack/mpls/mpls-routing-protocol.cc:79: this->SetNode (node);
make SetNode private then

http://codereview.appspot.com/850045/diff/1/23
File src/mpls-stack/mpls/mpls-routing-protocol.h (right):

http://codereview.appspot.com/850045/diff/1/23#newcode79
src/mpls-stack/mpls/mpls-routing-protocol.h:79: Ptr<NetDevice>
GetDeviceForMplsInterface (uint32_t ifIndex) const;
I see that you have both flavors of GetDevice because your indexes are not the
same as the ip stack indexes. I think that this would be fixed if you used the
Node::AddListener method I mention in my review later.

http://codereview.appspot.com/850045/diff/1/25
File src/mpls-stack/mpls/mpls-stack-helper.h (right):

http://codereview.appspot.com/850045/diff/1/25#newcode50
src/mpls-stack/mpls/mpls-stack-helper.h:50: void Assign (Ptr<NetDevice> device)
const;
It would be nice to get rid of these Assign methods by adding a
Node::RegisterDeviceAdditionListener method and calling it in
MplsRoutingProtocol::NotifyNewAggregate (called from Object::AggregateObject)

Here is an untested patch that adds this new Node method (I wrote it for some
unfinished code of mine):
http://www-sop.inria.fr/members/Mathieu.Lacage/node.patch

[Tom Henderson, 2010-05-22 06:12:43]

Overall this code looks good.  However, besides dealing with the recent review
comments, there are a few key missing pieces required to merge:  1) all public
methods documented with Doxygen, 2) some kind of overview documentation about
what this model is, what is its scope of applicability (what is implemented from
the RFCs and what is not); see src/routing/aodv/aodv.h for an example, 3) there
are no tests.  For tests, I would like to see at least one non-trivial test that
exercises the key elements of the code, including showing how MPLS can cause
packets to traverse the non-shortest-path from IP perspective, and handle both
IPv4 and IPv6 flows.  The test could go in src/test; I could help with setting
that up if needed.

http://codereview.appspot.com/850045/diff/1/3
File src/mpls-stack/examples/mpls1.cc (right):

http://codereview.appspot.com/850045/diff/1/3#newcode104
src/mpls-stack/examples/mpls1.cc:104: libtable.InstallEntry (  1,               
                  18, -1,  MplsLibEntry::POP         );
Agree with mathieu's comment about overloading this method; I would suggest to
have separate "Install*" methods for each type of entry, and avoid having to put
a magic -1 number on OutIf.

http://codereview.appspot.com/850045/diff/1/4
File src/mpls-stack/mpls/forwarding-equivalence-class.cc (right):

http://codereview.appspot.com/850045/diff/1/4#newcode37
src/mpls-stack/mpls/forwarding-equivalence-class.cc:37: return -1;
shouldn't return negative integer (not a uint32_t)

http://codereview.appspot.com/850045/diff/1/11
File src/mpls-stack/mpls/ipv4-host-address.h (right):

http://codereview.appspot.com/850045/diff/1/11#newcode39
src/mpls-stack/mpls/ipv4-host-address.h:39: static const uint32_t
FEC_MATCH_VALUE = 0x100;
Why is this hard coded?  Is this standard?  What if I come up with another FEC
such as Ipv4Tos-- what value do I put for the FEC_MATCH_VALUE?  If this value is
not standard, how about putting it in an attribute?

http://codereview.appspot.com/850045/diff/1/12
File src/mpls-stack/mpls/ipv4-to-mpls-routing-protocol.cc (right):

http://codereview.appspot.com/850045/diff/1/12#newcode88
src/mpls-stack/mpls/ipv4-to-mpls-routing-protocol.cc:88: case
MplsRoutingProtocol::ERROR_IPROUTING:
It may be a bit misleading to call this an error; usually this will just be
because it is the end of the MPLS tunnel

http://codereview.appspot.com/850045/diff/1/22
File src/mpls-stack/mpls/mpls-routing-protocol.cc (right):

http://codereview.appspot.com/850045/diff/1/22#newcode522
src/mpls-stack/mpls/mpls-routing-protocol.cc:522: // XXX: need MTU Path Discover
algoritm
should there be a drop trace for cases like this?

http://codereview.appspot.com/850045/diff/1/23
File src/mpls-stack/mpls/mpls-routing-protocol.h (right):

http://codereview.appspot.com/850045/diff/1/23#newcode80
src/mpls-stack/mpls/mpls-routing-protocol.h:80: Ptr<NetDevice>
GetDeviceForInterface (uint32_t ifIndex) const;
if you end up retaining both if indices, please do not call the parameters the
same name (use mplsIfIndex, for example)

http://codereview.appspot.com/850045/diff/1/24
File src/mpls-stack/mpls/mpls-stack-helper.cc (right):

http://codereview.appspot.com/850045/diff/1/24#newcode174
src/mpls-stack/mpls/mpls-stack-helper.cc:174: 
there is more code that is in your 1.1b1 repo.  Can you explain what is going on
for multicast (commented out there)?

[Tom Henderson, 2010-05-22 06:15:29]

On 2010/05/22 06:12:43, Tom Henderson wrote:
> Overall this code looks good.  However, besides dealing with the recent review
> comments, there are a few key missing pieces required to merge:  1) all public
> methods documented with Doxygen, 2) some kind of overview documentation about
> what this model is, what is its scope of applicability (what is implemented
from
> the RFCs and what is not); see src/routing/aodv/aodv.h for an example, 3)
there
> are no tests.  For tests, I would like to see at least one non-trivial test
that
> exercises the key elements of the code, including showing how MPLS can cause
> packets to traverse the non-shortest-path from IP perspective, and handle both
> IPv4 and IPv6 flows.  The test could go in src/test; I could help with setting
> that up if needed.

Forgot to mention, Wireshark can decode MPLS headers so it would be nice to
enable pcap on these flows and verify that Wireshark can read it.

[aachurin, 2010-05-22 11:22:41]

Thx!

I will modify the source in response to your comments in the near future


2010/5/22 <tomh.org@gmail.com>

> On 2010/05/22 06:12:43, Tom Henderson wrote:
>
>> Overall this code looks good.  However, besides dealing with the
>>
> recent review
>
>> comments, there are a few key missing pieces required to merge:  1)
>>
> all public
>
>> methods documented with Doxygen, 2) some kind of overview
>>
> documentation about
>
>> what this model is, what is its scope of applicability (what is
>>
> implemented from
>
>> the RFCs and what is not); see src/routing/aodv/aodv.h for an example,
>>
> 3) there
>
>> are no tests.  For tests, I would like to see at least one non-trivial
>>
> test that
>
>> exercises the key elements of the code, including showing how MPLS can
>>
> cause
>
>> packets to traverse the non-shortest-path from IP perspective, and
>>
> handle both
>
>> IPv4 and IPv6 flows.  The test could go in src/test; I could help with
>>
> setting
>
>> that up if needed.
>>
>
> Forgot to mention, Wireshark can decode MPLS headers so it would be nice
> to enable pcap on these flows and verify that Wireshark can read it.
>
>
> http://codereview.appspot.com/850045/show
>



-- 
Best regards,

Churin Andrey,
Lead Application Developer,
Vicman