[ovs-dev] [VLAN splinters 16/16] Implement new "VLAN splinters" feature.

[Ethan Jackson]

From: Ben Pfaff <blp at nicira.com>

It would be awesome if we could figure out some way to unit test this.

+    realdev = vsp_vlandev_to_realdev(ofproto, flow->in_port, &vid);
+    if (realdev) {
+        /* Cause the flow to be processed as if it came in on the real device
+         * with the VLAN device's VLAN ID. */
+        flow->in_port = realdev;

One thing makes me slightly nervous about this.  Suppose the flow table
attempts to output this packet to 'flow->in_port', will we allow that?  Perhaps
we'll need to keep track of the actual in_port separately?  I may be
misunderstanding how splinters work though.

+static int
+set_realdev(struct ofport *ofport_, uint16_t realdev_ofp_port, int vid)
+{
+    struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
+
+    if (realdev_ofp_port == ofport->realdev_ofp_port
+        && vid == ofport->vlandev_vid) {
+        return 0;
+    }
+
+    if (ofport->realdev_ofp_port) {
+        vsp_remove(ofport);
+    }
+    if (ofport->bundle) {
+        bundle_set(ofport->up.ofproto, ofport->bundle, NULL);
+    }
+
+    ofport->realdev_ofp_port = realdev_ofp_port;
+    ofport->vlandev_vid = vid;
+
+    if (realdev_ofp_port) {
+        vsp_add(ofport, realdev_ofp_port, vid);
+    }
+
+    return 0;
+}

Do we need to revalidate flows if the realdev_ofp_port of 'ofport_' changes?
Also, why are we clearing 'ofport_''s bundle's settings here?

+static uint32_t
+vsp_realdev_to_vlandev(const struct ofproto_dpif *ofproto,
+                   uint32_t realdev_odp_port, ovs_be16 vlan_tci)

The indentation is incorrect here.

+static struct vlan_splinter *
+vlandev_find(const struct ofproto_dpif *ofproto, uint16_t vlandev_ofp_port)
+{
+    const struct vlan_splinter *vsp;
+
+    HMAP_FOR_EACH_WITH_HASH (vsp, vlandev_node, hash_int(vlandev_ofp_port, 0),
+                             &ofproto->vlandev_map) {
+        if (vsp->vlandev_ofp_port == vlandev_ofp_port) {
+            return (struct vlan_splinter *) vsp;

Why not just drop the const from 'vsp' so you can avoid the cast?

+void
+ofproto_get_vlan_usage(const struct ofproto *ofproto,
+                       unsigned long int *vlan_bitmap)

This function could use a comment.

+int
+ofproto_port_set_realdev(struct ofproto *ofproto, uint16_t vlandev_ofp_port,
+                         uint16_t realdev_ofp_port, int vid)

As could this one.

+/* Returns true if VLAN splinters are enabled on 'iface_cfg', false
+ * otherwise. */
+static bool
+enable_vlan_splinters(const struct ovsrec_interface *iface_cfg)

I think something like "vlan_splinters_is_enabled" would be a better name for
this function.  The current name sounds like it will change the interfaces
configuration so that vlan splinters are enabled.


+    bitmap_set0(splinter_vlans, 0);
+    bitmap_set0(splinter_vlans, 4095);

It's not clear to me why we are unsetting these two vlans.  Perhaps a comment
would be helpful.

+                    if (!netdev_get_in4(netdev, NULL, NULL)) {
+                        vlandev_del(vlan_dev->name);
+                    } else {
+                        /* It has an IP address configured, so we don't own
+                         * it.  Don't delete it. */
+                    }

Is this going to break if they are using IPv6?