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

Reply via email to