Thanks again Ben for the review. Please see inline <rk>
-----Original Message-----
From: dev-boun...@openvswitch.org [mailto:dev-boun...@openvswitch.org] On
Behalf Of Ben Pfaff
Sent: Friday, March 09, 2012 11:21 AM
To: Kerur, Ravi
Cc: dev@openvswitch.org
Subject: [ovs-dev] MPLS less-important comments (was: Re: Patch for MPLS)
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.
<rk> Traditinally MPLS is treated as a L2.5. Would like to leave it like that.
This assignment is unnecessary because the top-level memset() will
already have zeroed mpls_lse:
} else {
flow->mpls_lse = htonl(0);
}
<rk> Removed the code.
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))) {
<rk> Removed it.
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);
}
<rk> Agreed and fixed it.
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 ## */
/* ## ---- ## */
<rk> Fixed it.
lib/nx-match.c
--------------
I think that nx_put_match() can use mpls_lse_to_label() and
mpls_lse_to_tc().
<rk> Agreed, fixed it.
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.
<rk> Agreed. It was added during debugging when things were not working. Fixed
it now.
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)) {
<rk> Fixed it.
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.
<rk> Fixed it.
In parse_mpls_onward(), the outer parentheses here are unnecessarily doubled:
+ expected_attrs |= ((UINT64_C(1) << OVS_KEY_ATTR_MPLS));
<rk> Fixed it.
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().
<rk> Fixed it.
lib/ofp-util.h
--------------
The OFP_ACTION_* enum values are only used in ofproto-dpif.c, so I'd
move them there.
<rk> Mainly used for mpls delayed action. Removed it.
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))
<rk> Userspace code is tested now and comments incorporated.
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.
<rk> Don't need them, removed.
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.
<rk> Mostly safer coding. Not needed now hence removed.
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.
<rk> I think it is true. If no label or tc is specified all packets which has
ethertype=0x8847/0x8848 is matched.
Thanks,
Ravi
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
