On Thu, Jun 2, 2016 at 3:47 PM, Thadeu Lima de Souza Cascardo
<casca...@redhat.com> wrote:
> 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
>> <casca...@redhat.com> wrote:
>> > 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?
I don't think there is a need to specialize this for VXLAN since I
would expect that the dpif_ports are already non overlapping for
different classes. I think the biggest question is whether it is
possible to have the same port name across different bridges. In
theory this should be possible but it might not be in practice.
>> > + 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.
I think you're right - I misread the comment. Of course, there is a
memory leak here related to the deletion of ports that you mentioned
before.
>> > 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?
I think just moving this above the configuration change should solve
the problem.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
