On Thu, Jun 16, 2011 at 1:05 PM, Ben Pfaff <[email protected]> wrote: > 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.
I guess although this seemed better as it is more consistent. By the way, my eventual plan is to convert the kernel actions to be pure push/pop for vlan tags as I think that really is the correct model and make userspace generate a pop and push pair to emulate the OpenFlow set action if necessary. >> > 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. Actually, I decided to just convert it to skb->protocol. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
