On Wed, Nov 23, 2011 at 01:26:31PM -0800, Ethan Jackson wrote:
> It would be awesome if we could figure out some way to unit test this.
Yeah. I'll see if I can find time for that. We'd need a way to dummy
up the vlan devices; probably that's not too hard.
> + 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.
You mean, if the flow table attempts to output to the splinter that it
came in on? Yeah, we don't do anything about that, and you'd end up
sending the packet out the in-port. The splinters currently show up
via OpenFlow. I'd rather hide them, but I didn't do the required work
or even scope it out.
It should work OK, in the expected manner, if the flow table attempts
to output to the real device.
> +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?
Yes, thanks. I added
ofproto->need_revalidate = true;
just after the initial "if" statement.
> Also, why are we clearing 'ofport_''s bundle's settings here?
I don't want vlandevs showing up in bundles.
I added a comment:
/* vlandevs are enslaved to their realdevs, so they are not allowed to
* themselves be part of a bundle. */
> +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.
Fixed, thanks.
> +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?
OK, done.
> +void
> +ofproto_get_vlan_usage(const struct ofproto *ofproto,
> + unsigned long int *vlan_bitmap)
>
> This function could use a comment.
OK, I added:
/* Sets a 1-bit in the 4096-bit 'vlan_bitmap' for each VLAN ID that is matched
* (exactly) by an OpenFlow rule in 'ofproto'. */
> +int
> +ofproto_port_set_realdev(struct ofproto *ofproto, uint16_t vlandev_ofp_port,
> + uint16_t realdev_ofp_port, int vid)
>
> As could this one.
I added:
/* Configures a VLAN splinter binding between the ports identified by OpenFlow
* port numbers 'vlandev_ofp_port' and 'realdev_ofp_port'. If
* 'realdev_ofp_port' is nonzero, then the VLAN device is enslaved to the real
* device as a VLAN splinter for VLAN ID 'vid'. If 'realdev_ofp_port' is zero,
* then the VLAN device is un-enslaved. */
> +/* 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.
Changed, thanks.
> + 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.
OK, I added:
/* Don't allow VLANs 0 or 4095 to be splintered. VLAN 0 should appear on
* the real device. VLAN 4095 is reserved and Linux doesn't allow a VLAN
* device to be created for it. */
> + 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?
Yes.
OK, I added a check for an IPv6 address too.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
