[ovs-dev] [PATCH v2 1/2] nx-match: Use vl_mff_map to parse match field.

[Yi-Hung Wei]

On Fri, Mar 10, 2017 at 11:06 AM, Joe Stringer <joe at ovn.org> wrote:

> On 9 March 2017 at 10:24, Yi-Hung Wei <yihung.wei at 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 at 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.