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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev