Re: [PATCH 5/6] openvswitch: Rename GENEVE_TUN_OPTS() to TUN_METADATA_OPTS()

On 01/07/15 at 02:46pm, Jesse Gross wrote:

On Tue, Jan 6, 2015 at 6:05 PM, Thomas Graf [off-list ref] wrote:

quoted

A subsequent patch will introduce VXLAN options. Rename the existing
GENEVE_TUN_OPTS() to reflect its extended purpose of carrying generic
tunnel metadata options.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

This is generally a good idea (even outside of the larger context of
this patch series) although a couple comments below:

quoted

diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index d1eecf7..c60ae3f 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c

@@ -387,20 +387,20 @@ static int parse_flow_nlattrs(const struct nlattr *attr,
        return __parse_flow_nlattrs(attr, a, attrsp, log, false);
 }

-static int genev_tun_opt_from_nlattr(const struct nlattr *a,
-                                    struct sw_flow_match *match, bool is_mask,
-                                    bool log)
+static int tun_md_opt_from_nlattr(const struct nlattr *a,
+                                 struct sw_flow_match *match, bool is_mask,
+                                 bool log)

I think this is a somewhat overzealous conversion. This function is
verifying OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS and as such has some
elements of Geneve protocol knowledge baked in (such as the check that
the options are a multiple of 4 bytes). It's also nice for the log
messages to indicate which netlink key is causing the problem and not
be too generic.

OK. I was planning on always padding vxlan_opts to 4 bytes as well but
I agree with you. It seems clearer to have distinct functions for each
and they can share code where it makes sense.

quoted

@@ -1148,10 +1145,10 @@ int ovs_nla_put_flow(const struct sw_flow_key *swkey,
                goto nla_put_failure;

        if ((swkey->tun_key.ipv4_dst || is_mask)) {
-               const struct geneve_opt *opts = NULL;
+               const void *opts = NULL;

                if (output->tun_key.tun_flags & TUNNEL_OPTIONS_PRESENT)
-                       opts = GENEVE_OPTS(output, swkey->tun_opts_len);
+                       opts = TUN_METADATA_OPTS(output, swkey->tun_opts_len);

                if (ipv4_tun_to_nlattr(skb, &output->tun_key, opts,
                                       swkey->tun_opts_len))

I think it's probably better to factor this block of code out into a
new function as you have done in the next patch (and include it with
this one). It makes it clearer that it is Geneve-specific.

OK, will do.