On Thu, Jun 2, 2016 at 3:18 AM, Thadeu Lima de Souza Cascardo
<casca...@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 <casca...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
