[ovs-dev] MPLS important comments (was: Re: Patch for MPLS)

[Ben Pfaff]

On Tue, Mar 13, 2012 at 08:13:11PM +0100, Ravi.Kerur at telekom.com wrote:
> Thanks again Ben. Comments inline <rk>

Why don't you quote in a normal way, with some kind of indentation?
These <rk> markers are hard to spot.

Most of your responses make perfect sense to me.  Thanks.

Can you post your current patch (patches, I guess) in a new thread, so
that I can be sure that I'm looking at the right ones?  You don't need
to rebase against master, as long as you let us know what the series
is based on.

> > It appears that push_mpls() refuses to add an MPLS header if there is
> > a VLAN tag and the inner Ethertype is not IPv4.  Why?
> > 
> > <rk> I didn't think any other L3 protocol would fit into the
> > use-case. For IPV6, it has its own label and I am not sure there
> > would be a case for <VLAN/IPV6> packet to push a new MPLS
> > label. Make sense? Or are you referring to other L3 protocol aka
> > IPX...?
> 
> Do you mean that there is a different Ethertype for IPv6-in-MPLS?  Or
> do you mean something different? 
> 
> <rk> I meant to say I don't know of any use case where MPLS shim
> header would be pushed on an IPv6 packet since IPv6 has its own flow
> label. Ethtype is the same for all layer-3 protocols. I will add
> IPv6 support with the assumption that there might be a use case
> which requires MPLS shim header for IPv6 packets. Please let me know
> if I have misunderstood your question.

I'm not sure.  I'm going to separately take this up with Bruce
Davies.  I bet that he can help me figure it out.  Thanks.

> > <rk> Now, a request. It's really hard to carry around this huge
> > change for a long time, because everytime I merge I see conflict in
> > different areas of code and have to retest it each time. If you
> > could let me push the modified code(I will send out updated patch
> > shortly) into targeted release it would really help me. I will work
> > on unit-tests shortly after the push and get back to you at the
> > earliest. I have made sure "make check" matches latest code in
> > master. Since new code is not breaking existing functionality it
> > would really help me if it finds a home.
> 
> We don't put unfinished code into our repository.  You can make your
> life easier, though, a couple of ways.  First, there's no need to
> merge or rebase often.  You don't even have to do it before sending
> out a revised patch, as long as you let us know what commit it's based
> on.  Second, if retesting is a problem then it means that the tests
> aren't automatic enough.  "make check TESTSUITEFLAGS=-j8" runs in 26
> seconds here, which is not a large burden.
> 
> <rk> I think we are not in agreement here. I am not saying testing
> existing OVS functionality is a burden. What I meant to say was mpls
> tests are not automated yet and I have to go through manual testing
> on 2 systems to make sure nothing is broken after every
> merge/rebase. It would be really helpful if I can just work on
> unit-tests as a separate patch such that it makes it easier and
> faster to test the mpls functionality. Furthermore, it doesn't break
> any existing functionality and hence I am making the case.

Sorry.  I cannot accept half-finished code.  I especially cannot take
changes to the datapath without Jesse's approval, and I don't have
that yet.

Thanks,

Ben.