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
> <[email protected]> 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 <[email protected]>
>
> 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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
