On Thu, Jun 16, 2011 at 12:51:21PM -0700, Jesse Gross wrote: > 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().
Fair enough. I also think it would acceptable to just add the VLAN in that case. The packet was nonsensical before; adding a VLAN won't make it more or less nonsensical. > > 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. OK. Thanks, Ben. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
