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

[Joe Stringer]

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.

> @@ -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.

>   * 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.