[ovs-dev] [PATCH 01/16] User-Space MPLS actions and matches

[Ben Pfaff]

On Fri, Jan 18, 2013 at 09:27:55AM +0900, Simon Horman wrote:
> This patch implements use-space datapath and non-datapath code
> to match and use the datapath API set out in Leo Alterman's patch
> "user-space datapath: Add basic MPLS support to kernel".

I'm resuming my review starting from packets.c.

eth_mpls_depth() seems poorly documented to me.  The comment makes an
incorrect claim about packet->l2, which the function does not use, and
does not mention packet->l2_5, which the function does use.

In get_ethtype_ptr(), the expression in the "return" looks wrong:
    if (eh->eth_type == htons(ETH_TYPE_VLAN)) {
        return (ovs_be16 *)((char *)(packet->l2_5 ? packet->l2_5 : packet->l3)) - 2;
because casts have higher precedence than subtraction, so I believe this
will subtract 2*sizeof(uint16) == 4 bytes from the beginning of the L2.5
or L3 header, which is wrong.  Do any tests cover this?

I believe that the correct name for an MPLS header is a "label", not a
"tag".  It would be nice to get this right, although certainly it's not
a huge deal.

set_mpls_lse_values() has ;; at the end of one line.

push_mpls_lse() isn't documented as putting the new MPLS label before or
after the existing labels, but it appears to me that it puts the new
label after the existing labels.  If I'm right about that, it's
surprising; one would ordinarily expect a new header to be an outermost
one, not an innermost one.

I'm still reading, I'll continue later.