On Thu, Jun 16, 2011 at 12:37 PM, Ben Pfaff <[email protected]> wrote: > On Wed, Jun 15, 2011 at 11:00:05AM -0700, Jesse Gross wrote: >> The sematics for setting a vlan tag are to modify the existing tag >> if one exists. This can be expressed as removing the existing tag >> first and then adding a new one. This simplifies the code by not >> requiring two copies of the logic that manipulates non-accelerated >> vlans and should not make a performance difference because the vlan >> tag is contained in a single cache line. >> >> Signed-off-by: Jesse Gross <[email protected]> > > Acked-by: Ben Pfaff <[email protected]> > > But the test for VLAN_ETH_HLEN isn't needed, I think, because > strip_vlan() does the same test.
The issue is that if the length is not sufficient for a vlan tag then strip_vlan() returns the skb, which is the same as what it does on success and we will proceed to add a new vlan. By checking first we won't add a new vlan header and will immediately return. This is what we do for other actions and is consistent with the previous behavior for modify_vlan_tci(). > strip_vlan() checks vlan_eth_hdr(skb)->h_vlan_proto for 802.1Q > whereas modify_vlan_tci() checks skb->protocol. I don't know if > consistency is important. I think that skb->protocol is the more canonical way to do it but I can't think of a situation in which vlan_eth_hdr(skb)->h_vlan_proto would be different, so I didn't bother to change it. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
