[ovs-dev] [PATCH 2/3] netdev-vport: reject concomitant incompatible tunnels

[Jesse Gross]

On Thu, Jun 2, 2016 at 3:18 AM, Thadeu Lima de Souza Cascardo
<cascardo at redhat.com> wrote:
> If VXLAN-GBP and VXLAN are set on the same port, only one of those tunnel types
> is going to be created. As other tunnel flags are added, like VXLAN-GPE, and
> they are incompatible, reject any configuration that put incompatible tunnels on
> the same port.
>
> A manual test has been done as well and the port didn't get an ofport. During a
> restart, the other tunnel type was rejected and the right flags were on the
> dpif_port.
>
> Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo at redhat.com>

I saw your follow up to yourself but I already started looking at
this, so I might as well give the comments that I have.

> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 6016fd8..aaf6bfc 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -38,6 +38,7 @@
>  #include "packets.h"
>  #include "poll-loop.h"
>  #include "route-table.h"
> +#include "simap.h"
>  #include "smap.h"
>  #include "socket-util.h"
>  #include "unaligned.h"
> @@ -63,6 +64,7 @@ static bool tunnel_check_status_change__(struct netdev_vport *);
>  struct vport_class {
>      const char *dpif_port;
>      struct netdev_class netdev_class;
> +    struct simap dst_port_to_exts;
>  };

I don't think it's really makes sense to have a per-class map. Across
classes, I would always expect for dpif_ports to be unique but they
might not for the same class on difference dpif backers (i.e. kernel
vs. DPDK - though I'm not sure that we really handle this case very
well right now). Plus I don't really like the need to pass in the
array index of the vport_class when initializing these things.

> +static bool
> +netdev_vport_may_add(struct netdev *netdev)
> +{

I would give this a name that is a little less generic and more
specifically reflects what is it

> +    char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
> +    const char *name;
> +    struct simap_node *node;
> +    struct vport_class *vclass;
> +    const struct netdev_vport *vport;
> +    const struct netdev_class *class = netdev_get_class(netdev);
> +
> +    if (!is_vport_class(class)) {
> +        return false;
> +    }
> +
> +    if (!netdev_vport_needs_dst_port(netdev)) {
> +        return true;
> +    }
> +
> +    vport = netdev_vport_cast(netdev);
> +    vclass = vport_class_cast(class);
> +    name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
> +
> +    node = simap_find(&vclass->dst_port_to_exts, name);
> +    if (!node) {
> +        simap_put(&vclass->dst_port_to_exts, name, vport->tnl_cfg.exts);

When name is added to the simap, it is a reference to the one that is
passed in - which in this case is just on the stack.

>  static int
>  set_tunnel_config(struct netdev *dev_, const struct smap *args)
>  {
> @@ -615,6 +647,10 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args)
>      }
>      ovs_mutex_unlock(&dev->mutex);
>
> +    if (!netdev_vport_may_add(dev_)) {
> +        return EEXIST;
> +    }

Presumably this check should come before we update the configuration
for existing tunnels.