| Message ID | CAPWQB7F12iBs3F_2_pLr-WoVWQPTzzASUZrW7VfLnJu7Ua+4Ug@mail.gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Fri, Mar 10, 2017 at 11:06 AM, Joe Stringer <joe@ovn.org> wrote: > On 9 March 2017 at 10:24, Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > vl_mff_map is introduced in commit 04f48a68c428 ("ofp-actions: Fix > variable > > length meta-flow OXMs") to account variable length mf_field, and it is > used > > to decode variable length mf_field in ofp_action. In this patch, > vl_mff_map > > is further used to decode the variable length match field as well. > > > > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> > > Thanks for v2, I had a bit of trouble applying to master but I've > reviewed below anyway. I think that Jarno's changes yesterday > conflict, so you'll need to rebase before submitting the next version. > Thanks, I will send out a v3 based on this review. > > > @@ -594,16 +598,21 @@ nx_pull_match__(struct ofpbuf *b, unsigned int > match_len, bool strict, > > * are valid pointers, then stores the cookie and mask in them if 'b' > contains > > * a "NXM_NX_COOKIE*" match. Otherwise, stores 0 in both. > > * > > + * 'vl_mff_map" is an optional parameter that is used to derive > variable length > > + * mf_fields in flow match. If it is not provided, the default > mf_fields with > > + * maximum length will be used. > > + * > > Would the below be more accurate and informative? Or is it too specific? > > 'vl_mff_map' is an optional parameter that is used to validate the > length of variable > length mf_fields in 'match'. If it is not provided, the default > mf_fields with maximum > length will be used. > > I guess there's a couple of places where this comment exists that > would benefit from this. > Thanks, it's more clear. I applied that to v3. > > > * Fails with an error upon encountering an unknown NXM header. > > * > > * Returns 0 if successful, otherwise an OpenFlow error code. */ > > enum ofperr > > nx_pull_match(struct ofpbuf *b, unsigned int match_len, struct match > *match, > > ovs_be64 *cookie, ovs_be64 *cookie_mask, > > - const struct tun_table *tun_table) > > + const struct tun_table *tun_table, > > + const struct vl_mff_map *vl_mff_map) > > { > > return nx_pull_match__(b, match_len, true, match, cookie, > cookie_mask, > > - tun_table); > > + tun_table, vl_mff_map); > > } > > > > /* Behaves the same as nx_pull_match(), but skips over unknown NXM > headers, > > @@ -612,15 +621,17 @@ enum ofperr > > nx_pull_match_loose(struct ofpbuf *b, unsigned int match_len, > > struct match *match, > > ovs_be64 *cookie, ovs_be64 *cookie_mask, > > - const struct tun_table *tun_table) > > + const struct tun_table *tun_table, > > + const struct vl_mff_map *vl_mff_map) > > { > > return nx_pull_match__(b, match_len, false, match, cookie, > cookie_mask, > > - tun_table); > > + tun_table, vl_mff_map); > > } > > Now that I look at the comment above these *_loose() functions, I > wonder - should the vl_mff_map ever be specified for these versions? > > If I follow correctly, these loose functions are useful for decoupling > controller and switch support for fields. For example, if the switch > sends up something in a slightly different format than what the > controller expects, the controller is able to basically skip over the > unknown bits but still process the rest of the message. One case might > be a packet_in message from the switch to the controller. The switch > would provide packet and its view of the parsed packet to the > controller, but the controller doesn't have to work with the same > knowledge of all packet fields as what the switch understands. Should > OXM lengths be allowed to be flexible when decoding in a controller? > Well, I guess that the controller should be responsible for ensuring > that it configures the TLV map correctly ahead of time.. so if the > controller already has this knowledge, then it's not unreasonable for > it to keep its own view of the TLV maps in sync with the switch. On > the flip side, we enforce that the controller must maintain the same > vll mff map as what the switch has. > > I discussed this briefly with Jarno (thanks!), and figured it would be > easy enough to try this out.. > > diff --git a/lib/nx-match.c b/lib/nx-match.c > index 1a0f0de594e7..f1e020a766c6 100644 > --- a/lib/nx-match.c > +++ b/lib/nx-match.c > @@ -624,6 +624,8 @@ nx_pull_match_loose(struct ofpbuf *b, unsigned int > match_len, > const struct tun_table *tun_table, > const struct vl_mff_map *vl_mff_map) > { > + ovs_assert(!tun_table); > + ovs_assert(!vl_mff_map); > return nx_pull_match__(b, match_len, false, match, cookie, cookie_mask, > tun_table, vl_mff_map); > } > > When I ran the OVS testsuite with this applied, all the tests ran > successfully so it seems to me that it would be safe not to add the > extra vl_mff_map argument to these *_loose() functions. It would be > good if you could later on submit a separate patch to remove tun_table > argument from these as well. > Thanks, I remove vl_mff_map from nx_pull_match_loose() and oxm_pull_match_loose(). I also add a patch that modifies oxm_decode_match_loose() to oxm_decode_match() that takes decoding NXT_RESUME into account while we decoding oxm match.
diff --git a/lib/nx-match.c b/lib/nx-match.c index 1a0f0de594e7..f1e020a766c6 100644 --- a/lib/nx-match.c +++ b/lib/nx-match.c @@ -624,6 +624,8 @@ nx_pull_match_loose(struct ofpbuf *b, unsigned int match_len, const struct tun_table *tun_table, const struct vl_mff_map *vl_mff_map) { + ovs_assert(!tun_table); + ovs_assert(!vl_mff_map); return nx_pull_match__(b, match_len, false, match, cookie, cookie_mask, tun_table, vl_mff_map);