Thanks, I look forward to the revised patch.
On Mon, Mar 12, 2012 at 10:53:05PM +0100, ravi.ke...@telekom.com wrote: > 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
