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