[ovs-dev] [PATCH v2] Allow configuration of null ports (flags, dst_port).

[Jesse Gross]

On Fri, Dec 21, 2012 at 7:44 AM, Jarno Rajahalme
<jarno.rajahalme at nsn.com> wrote:
> Make all kernel tunnel configuration attributes optional. Attributes
> meaningful for null ports are processed first, and the rest are skipped for
> null ports.
>
> Signed-off-by: Jarno Rajahalme <jarno.rajahalme at nsn.com>
> ---
>
> v2 makes also flags optional, avoids setting any key flags for null ports,
> and still requires remote IP to be configured on the API level as null ports
> are not yet functional.
>
> Not sure if any of the flags are useful for a null port, but it seems
> that at least the TNL_F_IPSEC could to be still needed.

The plan is to start using skb mark for interaction with the IPsec
stack.  This should be a little more flexible and provide better
interaction with other utilities.  As a result, I don't think that any
of the flags are necessary in the null port case and we can skip
processing of those as well.

We probably also want to unconditionally return the flags in
ovs_tnl_get_options() in the non-null port case again for
compatibility reasons.  Similarly, we probably should unconditionally
send the flags from userspace (which is implicitly a non-null port
case).

Otherwise, I just have a few minor suggestions.

> diff --git a/datapath/tunnel.c b/datapath/tunnel.c
> index 26d9014..97c3d8e 100644
> --- a/datapath/tunnel.c
> +++ b/datapath/tunnel.c
> @@ -1067,11 +1067,22 @@ static int tnl_set_config(struct net *net, struct nlattr *options,
>         if (err)
>                 return err;
>
> -       if (!a[OVS_TUNNEL_ATTR_FLAGS] || !a[OVS_TUNNEL_ATTR_DST_IPV4])
> -               return -EINVAL;
> +       /* Process attributes possibly useful for null_ports first */
> +       if (a[OVS_TUNNEL_ATTR_FLAGS])
> +               mutable->flags = nla_get_u32(a[OVS_TUNNEL_ATTR_FLAGS])
> +                       & TNL_F_PUBLIC;
> +
> +       if (a[OVS_TUNNEL_ATTR_DST_PORT])
> +               mutable->dst_port =
> +                       htons(nla_get_u16(a[OVS_TUNNEL_ATTR_DST_PORT]));
>
> -       mutable->flags = nla_get_u32(a[OVS_TUNNEL_ATTR_FLAGS]) & TNL_F_PUBLIC;
> -       mutable->key.daddr = nla_get_be32(a[OVS_TUNNEL_ATTR_DST_IPV4]);
> +       if (a[OVS_TUNNEL_ATTR_DST_IPV4]) {
> +               mutable->key.daddr = nla_get_be32(a[OVS_TUNNEL_ATTR_DST_IPV4]);
> +       }

Usually the preferred kernel style is to not use braces in the case of
a single line if statement.

> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 2b493a7..9cf0906 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -162,7 +162,8 @@ netdev_vport_get_netdev_type(const struct dpif_linux_vport *vport)
>                                          a)) {
>              break;
>          }
> -        return (nl_attr_get_u32(a[OVS_TUNNEL_ATTR_FLAGS]) & TNL_F_IPSEC
> +        return (a[OVS_TUNNEL_ATTR_FLAGS]
> +                && nl_attr_get_u32(a[OVS_TUNNEL_ATTR_FLAGS]) & TNL_F_IPSEC
>                  ? "ipsec_gre" : "gre");

I would add a function to handle the test for attribute not present
since we have three copies of it.  We have get_be64_or_zero() already
so we could add an equivalent for u32.

> @@ -538,12 +540,13 @@ netdev_vport_get_tnl_iface(const struct netdev *netdev)
>                                      a)) {
>          return NULL;
>      }
> -    route = nl_attr_get_be32(a[OVS_TUNNEL_ATTR_DST_IPV4]);
> +    if (a[OVS_TUNNEL_ATTR_DST_IPV4]) {
> +        route = nl_attr_get_be32(a[OVS_TUNNEL_ATTR_DST_IPV4]);

We could drop the definition of route down into this if block now
since it's only used there.

> @@ -743,6 +746,8 @@ parse_tunnel_config(const char *name, const char *type,
>      set_key(args, "in_key", OVS_TUNNEL_ATTR_IN_KEY, options);
>      set_key(args, "out_key", OVS_TUNNEL_ATTR_OUT_KEY, options);
>
> +    /* Remote_ip requirement will be removed when flow-based tunneling
> +     * is complete. */
>      if (!daddr) {
>          VLOG_ERR("%s: %s type requires valid 'remote_ip' argument",
>                   name, type);

This comment is potentially a little confusing since there are
multiple phases of flow based tunneling.  The first one is entirely
internal to userspace/kernel and won't be visible to the outside world
yet.