On Tue, Nov 01, 2011 at 10:50:51PM -0700, Jesse Gross wrote:
> In the future it is likely that our vlan support will expand to
> include multiply tagged packets. When this happens, we would
> ideally like for it to be consistent with our current tagging.
>
> Currently, if we receive a packet with a partial VLAN tag we will
> automatically drop it in the kernel, which is unique among the
> protocols we support. The only other reason to drop a packet is
> a memory allocation error. For a doubly tagged packet, we will
> parse the first tag and indicate that another tag was present but
> do not drop if the second tag is incorrect as we do not parse it.
>
> This changes the behavior of the vlan parser to match other protocols
> and also deeper tags by indicating the presence of a broken tag with
> the 802.1Q EtherType but no vlan information. This shifts the policy
> decision to userspace on whether to drop broken tags and allows us to
> uniformly add new levels of tag parsing.
>
> Although additional levels of control are provided to userspace, this
> maintains the current behavior of dropping packets with a broken
> tag when using the NORMAL action because that is the correct behavior
> for an 802.1Q-aware switch. The userspace flow parser actually
> already had the new behavior so this corrects an inconsistency.
>
> Signed-off-by: Jesse Gross <[email protected]>
It took me a minute to realize that by "partial VLAN tag" you mean a
frame that is truncated after the tpid.
A frame with a partial VLAN tag can't have anything after the partial
tag, but the kernel flow_from_nlattrs() appears to allow arbitrary
additional flow data, e.g. it will accept a partial tag followed by a
valid Ethertype, IP, etc.
Representing a partial VLAN tag as a special case of
OVS_KEY_ATTR_8021Q seems to work, but I'm inclined to think that just
using OVS_KEY_ATTR_ETHERTYPE would work just as well. After all, a
VLAN tag without a TCI isn't really a VLAN tag at all, so I don't know
that it necessarily makes sense to represent it as one, but it's
otherwise a perfectly good Ethertype. I think that just using
OVS_KEY_ATTR_ETHERTYPE would introduce fewer special cases elsewhere
in the tree, too.
It's probably a good idea to distinguish these special non-tags in
format_odp_key_attr(), otherwise someday I'll be very confused
debugging. And we'd want to adapt parse_odp_key_attr() also to be
able to parse them.
I think that the changes to odp_flow_key_from_flow() are unnecessary.
As posted, I think that they won't have the desired effect of creating
partial VLAN tags, but I believe that the original code would have.
Do you want the {} around the statement below?
> @@ -949,9 +953,13 @@ int flow_from_nlattrs(struct sw_flow_key *swkey, int
> *key_lenp,
> /* Only standard 0x8100 VLANs currently supported. */
> if (q_key->q_tpid != htons(ETH_P_8021Q))
> goto invalid;
> - if (q_key->q_tci & htons(VLAN_TAG_PRESENT))
> - goto invalid;
> - swkey->eth.tci = q_key->q_tci | htons(VLAN_TAG_PRESENT);
> + if (q_key->q_tci & htons(VLAN_TAG_PRESENT)) {
> + swkey->eth.tci = q_key->q_tci;
> + } else {
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
