On Mon, Oct 15, 2012 at 10:40:12AM -0700, Ben Pfaff wrote:
> 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?
I have removed the ttl functions above, they are not used at this time.
>
> 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.
Yes. I believe this is fixed by the stack depth tracking change
that Yamahata-san suggested.
> 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().
I have dropped the change for REG_LOAD in do_xlate_actions(),
it seems spurious at this point.
> The tests don't actually run the test-mpls program. Let's just get
> rid of it.
I will post a fresh version of the MPLS series that addresses
all of the issues you have raised above.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
