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