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

[Thadeu Lima de Souza Cascardo]

On Thu, Jun 02, 2016 at 12:46:04PM -0700, Jesse Gross wrote:
> 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.

Hi, Jesse.

Thanks for the review and for applying the other patches. Some comments below.

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

I didn't like it either. It wasn't needed as I was calling simap_init and could
use a simple {} initializer, bug GCC 4.8 didn't like it.

Do you agree with using a single map for VXLAN, as it's the only one with a
problem as of now?

> > +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
> 

Commenting seems an important thing as well. I will think of something.

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

Oops. Not sure how that would have worked then. So, I looked at the code and
simap_put calls xmemdup0, which makes sense since it can also be used for a
replace.

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

So right before overwriting the configuration. Or did you think of something
more high level?

Again, thanks for the review.
Cascardo.