Thanks, Ravi. Here's some initial feedback. I didn't make it all the
way through the patch yet.
It applies, compiles, and passes "sparse" cleanly. Great!
I don't see any handling for decrementing a TTL of 0. That's supposed
to be sent to the controller with OFPR_INVALID_TTL.
include/linux/openvswitch.h
---------------------------
I think these should say be16 and be32 since their operands are
in network byte order:
+ OVS_ACTION_ATTR_PUSH_MPLS, /* u16, ethertype */
+ OVS_ACTION_ATTR_POP_MPLS, /* u16, ethertype */
+ OVS_ACTION_ATTR_SET_MPLS_LSE, /* u32, mpls label stack entry */
include/openflow/nicira-ext.h
-----------------------------
You can drop this line, it doesn't fit in in the context:
+ /* MPLS related definitions*/
lib/dpif-netdev.c
-----------------
I don't think there's a benefit to the temporary vars here:
+ case OVS_ACTION_ATTR_PUSH_MPLS:
+ push_ethertype = nl_attr_get_be16(a);
+ push_mpls(packet, push_ethertype);
+ break;
+
+ case OVS_ACTION_ATTR_POP_MPLS:
+ pop_ethertype = nl_attr_get_be16(a);
+ pop_mpls(packet, pop_ethertype);
+ break;
+
+ case OVS_ACTION_ATTR_SET_MPLS_LSE:
+ mpls_lse = nl_attr_get_be32(a);
+ set_mpls_lse(packet, mpls_lse);
+ break;
lib/flow.c
----------
In flow_extract(), I don't think the cast is necessary:
+ packet->l2_5 = (uint32_t *)b.data;
lib/odp-util.c
--------------
I think that these should be ovs_be<N> types:
+ case OVS_ACTION_ATTR_PUSH_MPLS: return sizeof(uint16_t);
+ case OVS_ACTION_ATTR_POP_MPLS: return sizeof(uint16_t);
+ case OVS_ACTION_ATTR_SET_MPLS_LSE: return sizeof(uint32_t);
format_odp_action() and parse_odp_action() use different syntax for
push_mpls and pop_mpls (one includes "tpid=", the other doesn't).
Please make them consistent.
(Is 'tpid' correct terminology for MPLS? I thought it was
VLAN-specific.)
In parse_odp_action(), the numbers passed to strncmp() and returned
differ, but they should be the same.
Code like this appears in a few places. Should we create a helper
function?
+ htonl((mpls_label << MPLS_LABEL_SHIFT) |
+ (mpls_tc << MPLS_TC_SHIFT) |
+ (mpls_ttl << MPLS_TTL_SHIFT) |
+ (mpls_stack << MPLS_STACK_SHIFT)));
The set_mpls_lse_values() function could also be implemented in terms
of such a helper.
I'd like to see commit_mpls_action() split into a function to pop an
MPLS header and a function to push an MPLS header. The current
organization is confusing and I'm not convinced that it's 100%
correct.
lib/packets.c
-------------
In set_mpls_lse_ttl(), please drop the unneeded parentheses here:
+ *tag |= (ttl & htonl(MPLS_TTL_MASK));
also in the other similar functions.
I think the logic in dec_mpls_ttl() only works because the TTL is the
low 8 bits of mpls_lse. Conceptually there's a missing shift.
copy_mpls_ttl_in() and copy_mpls_ttl_out() don't appear to check that
packet data is actually present.
Also in those functions, missing {} here:
+ if (mh == NULL || nh == NULL)
+ return;
The "return;" at the end of push_mpls_lse() may be omitted.
push_mpls() and push_mpls_lse() look like they don't check that IPv4
packet data is actually present.
I think it's time to add ofpbuf_*() functions to insert and remove
data in the middle of a packet. It's hard to see whether open-coded
moves (there are a few in this file in addition to the ones you added
for MPLS) are correct, so it'd be nice to get it right once and reuse
it.
ofproto/ofproto-dpif.c
----------------------
facet_account() has some stray VLOG_DBG calls.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
