On 01/20/2016 06:22 PM, Ben Pfaff wrote:
> The new logic here in physical_run() looks wrong to me. I think that it
> strips a vlan if it didn't match on one.
>
> if (tag) {
> match_set_dl_vlan(&match, htons(tag));
> } else if (!strcmp(binding->type, "localnet")) {
> /* Match priority-tagged frames, e.g. VLAN ID 0.
> *
> * We'll add a second flow for frames that lack any 802.1Q
> * header later. */
> match_set_dl_tci_masked(&match, htons(VLAN_CFI),
> htons(VLAN_VID_MASK | VLAN_CFI));
> }
>
> /* Strip vlans. */
> if (tag) {
> ofpact_put_STRIP_VLAN(&ofpacts);
> }
>
> I believe that we can simplify it to the following, while fixing the bug
> at the same time:
>
> /* Match a VLAN tag and strip it, including stripping priority
> tags
> * (e.g. VLAN ID 0). In the latter case we'll add a second flow
> * for frames that lack any 802.1Q header later. */
> if (tag || !strcmp(binding->type, "localnet")) {
> match_set_dl_vlan(&match, htons(tag));
> ofpact_put_STRIP_VLAN(&ofpacts);
> }
This makes sense to me, as long as these two are functionally
equivalent, because otherwise we'd be changing behavior from the current
code.
match_set_dl_vlan(&match, 0);
match_set_dl_tci_masked(&match, htons(VLAN_CFI),
htons(VLAN_VID_MASK | VLAN_CFI));
+--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
