Here are my additional comments that point out less-important issues.
include/openflow/nicira-ext.h
-----------------------------
The "format' in the definition of mpls_label isn't entirely clear to
me:
+/* The mpls_label in MPLS shim header.
+ *
+ * Prereqs: NXM_OF_ETH_TYPE must be either 0x8847 or 0x8848.
+ *
+ * Format: 32-bit integer, higher 20 bits
+ *
+ * Masking: Not maskable. */
The "format" in OF1.2 looks clearer to me, can you use that instead?
Here it is:
* Format: 32-bit integer in network byte order with 12 most-significant
* bits forced to 0. Only the lower 20 bits have meaning.
Similarly for mpls_tc:
* Format: 8-bit integer with 5 most-significant bits forced to 0.
* Only the lower 3 bits have meaning.
lib/flow.c
----------
parse_mpls() is unused. Please either use it or remove it.
I wonder whether we really need separate L2.5 and L3 in struct ofpbuf,
since if MPLS is present then we never parse past it. We could treat
MPLS as L3, if that is appropriate.
This assignment is unnecessary because the top-level memset() will
already have zeroed mpls_lse:
} else {
flow->mpls_lse = htonl(0);
}
You can drop the parentheses around the == here, and in practice we
normally do:
- if (eth->eth_type == htons(ETH_TYPE_VLAN)) {
+ if ((eth->eth_type == htons(ETH_TYPE_VLAN_8021q)) ||
+ (eth->eth_type == htons(ETH_TYPE_VLAN_8021ad))) {
This code in flow_zero_wildcards() looks odd to me:
if ((wc & FWW_MPLS_LABEL) || (wc & FWW_MPLS_TC)) {
if (wc & FWW_MPLS_LABEL) {
flow->mpls_lse &= ~htonl(MPLS_LABEL_MASK);
}
flow->mpls_lse &= ~htonl(MPLS_TTL_MASK);
flow->mpls_lse &= ~htonl(MPLS_STACK_MASK);
if (wc & FWW_MPLS_TC) {
flow->mpls_lse &= ~htonl(MPLS_TC_MASK);
}
} else {
flow->mpls_lse &= ~htonl(MPLS_TTL_MASK);
flow->mpls_lse &= ~htonl(MPLS_STACK_MASK);
}
I think that you can write it as just:
flow->mpls_lse &= ~htonl(MPLS_TTL_MASK | MPLS_STACK_MASK);
if (wc & FWW_MPLS_LABEL) {
flow->mpls_lse &= ~htonl(MPLS_LABEL_MASK);
}
if (wc & FWW_MPLS_TC) {
flow->mpls_lse &= ~htonl(MPLS_TC_MASK);
}
lib/meta-flow.c
---------------
Since each of the new fields has a width in bits less than its width
in bytes, it would be good to add them to the comment titled "Size"
within struct mf_field.
Please fix the box. Instead of this:
/* ## -- ## */
/* ## L2.5 ## */
/* ## -- ## */
write:
/* ## ---- ## */
/* ## L2.5 ## */
/* ## ---- ## */
lib/nx-match.c
--------------
I think that nx_put_match() can use mpls_lse_to_label() and
mpls_lse_to_tc().
lib/odp-util.h
--------------
I don't think that ODPUTIL_FLOW_KEY_BYTES or the comment above it
needs an update, because MPLS is always the last header parsed; that
is, it will never appear along with the IPv6, ICMPv6, and ND headers
in the list.
In parse_odp_key_attr() the outer parentheses here are unnecessarily doubled:
+ if ((sscanf(s, "mpls(label=%"SCNi32",tc=%i,ttl=%i)%n",
+ &mpls_label, &mpls_tc, &mpls_ttl, &n) > 0 && n > 0)) {
In parse_mpls_onward(), I think that the call to parse_l3_onward() can
change to just check_expectations(), because none of the "if"
statements in parse_l3_onward() will ever successfully match.
In parse_mpls_onward(), the outer parentheses here are unnecessarily doubled:
+ expected_attrs |= ((UINT64_C(1) << OVS_KEY_ATTR_MPLS));
lib/ofp-parse.c
---------------
In parse_named_action(), it would probably be a good idea to validate
that the arguments to set_mpls_label, set_mpls_tc, and set_mpls_ttl
are in the correct range. For push_mpls and pop_mpls, you can use
str_to_u16().
lib/ofp-util.c
--------------
A line is mis-indented in ofputil_normalize_rule().
lib/ofp-util.h
--------------
The OFP_ACTION_* enum values are only used in ofproto-dpif.c, so I'd
move them there.
lib/packets.c
-------------
The comment on set_mpls_lse_ttl() is wrong.
I think that the set_mpls_lse_ttl() code is only correct because
MPLS_TTL_SHIFT is 0.
dec_mpls_ttl() , copy_mpls_ttl_in(), and copy_mpls_ttl_out() could use
mpls_lse_to_ttl().
The first and final cases in push_mpls() are identical except for the
source of the TTL. I suggest using a helper function.
In pop_mpls(), you can write:
if ((ntohl(mh->mpls_lse) & MPLS_STACK_MASK) >> MPLS_STACK_SHIFT) {
as
if (mh->mpls_lse & htonl(MPLS_STACK_MASK))
lib/packets.h
-------------
Please don't use an OFP_ prefix for constants that aren't defined by
OpenFlow. I don't see OFP_MPLS_ANY or OFP_MPLS_NONE in OpenFlow. I
don't see any user of OFP_MPLS_ANY anywhere in the tree, either (where
is it used?)
If you do need them, please use #defines for these, because enum
constants always have type "int" which means that these are actually
negative values.
ofproto/ofproto-dpif.c
----------------------
The change to get_features() is a no-op, please drop it from the
patch.
I don't think the facet_account() changes actually have any visible
effect.
Why do all the "compose_*_mpls" functions check for null ctx or
ofproto? Can either one ever happen? All of these functions are
one-liners, and they only have a single caller, so I'd inline them
into their caller.
utilities/ovs-ofctl.8.in
------------------------
I don't think the sentence that starts "If none specified" below is
correct:
+.IP \fBmpls_label=\fIlabel\fR
+Matches MPLS Label when \fIethertype\fR is \fI0x8847\fR or \fI0x8848\fR.
+Specify a number between 16 and 2^20-1, inclusive, as the 20-bit MPLS label
+to match. If none specified, all packets which has \fIethertype\fR equal to
+\fI0x8847\fR or \fI0x8848\fR are matched.
The shorthand notation "mpls" (for dl_type=0x8847) should be mentioned
in the list of shorthands.
s/classe/class/ here:
+Modifies the MPLS traffic-classe of a packet.
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
