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

[Thomas Graf <tgraf@suug.ch>]

On 01/07/15 at 02:46pm, Jesse Gross wrote:
> On Tue, Jan 6, 2015 at 6:05 PM, Thomas Graf <tgraf@suug.ch> wrote:
> > 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:
> 
> > 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.

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