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

[Ben Pfaff]

set_ethertype() states:
        /* ethtype for VLAN packets is at L3_offset - 2 bytes. */
but I believe that this is also true for non-VLAN packets, possibly
allowing the code to be simplified.  Actually I guess that's not true
if there's an L2.5 header.

I think that the location of the ethertype is, then:
        just before the L2.5 header, if there is one;
        otherwise just before the L3 header.
get_ethertype() has essentially this logic, but if we wrote a function
that returns a pointer to the ethertype, then set_ethertype() and
get_ethertype() could be one-liners written in terms of that function.

In get_label_ttl_and_tc(), I don't think that the packet->size
comparisons are entirely correct, because one must allow for VLAN and
MPLS headers in the size calculations.  I think it would be better to
compute the size past the l3 pointer, e.g. something like
ofpbuf_tail(packet) - packet->l3 although there'd need to be some
casts.

I think that the get_label_ttl_and_tc() interface would be easier to
use if it just returned an MPLS label.  The caller can "or" in a BOS
bit after it returns, or the caller could provide a BOS value for
get_label_ttl_and_tc() to use.

I think set_new_mpls_lse() can be removed.  I don't see any benefit to
it over setting the mpls_lse member directly.

copy_mpls_ttl_in() duplicates code from packet_set_ipv4() for setting
the TTL.  It would be better to factor this out into a helper
function.

It's not obvious to me that copy_mpls_ttl_in() and copy_mpls_ttl_out()
are picky enough about packet length.

copy_mpls_ttl_out() has a comment that I don't understand
    /* TTL sent from ofproto-dpif.c is not the correct one,
     * hence ignore it. */

copy_mpls_ttl_in() and copy_mpls_ttl_out() both have comments that
->l2_5 must point to an MPLS header, but they still check for an MPLS
ethertype.  Is the check unnecessary, or is the comment wrong, or is
something else at work?

pop_mpls() has an "if" without {} around its statement.

The IP6_TC macro that this introduces is not used anywhere.  It seems
to me that, if we need this, it would be better written as a function
so as to emphasize that its parameter must be in host byte order.

There is a spurious change to subfacet_should_install().

The new assertion in execute_controller_action() makes me say "ugh".
I'd rather just delete it, I think.

Won't execute_controller_action() always push on an extra MPLS label,
if there's to be one at all?  Presumably it should just modify the
existing label if there is one.

compose_mpls_push_action() would seem to be a good place to use
set_mpls_lse_values().

In do_xlate_actions(), we could get rid of a couple of lines and a
variable simply by calling ofpact_get_REG_LOAD() inside the argument
of nxm_execute_reg_load().

The tests don't actually run the test-mpls program.  Let's just get
rid of it.

Thanks,

Ben.