On Thu, Jan 21, 2016 at 03:30:50PM -0500, Russell Bryant wrote:
> 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));
They're equivalent; you can find that out, eventually, by looking at
what match_set_dl_vlan() does.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
