[ovs-dev] [PATCH 1/4] classifier, flow : Set CFI as part of VID

Ben Pfaff blp at nicira.com
Wed Jul 4 21:55:52 UTC 2012


On Wed, Jul 04, 2012 at 05:50:41PM +0900, Simon Horman wrote:
> PCP depends on the presence of VID so it seems to make sense
> to set the CFI bit as part of setting the VID rather than the PCP.
> 
> Signed-off-by: Simon Horman <horms at verge.net.au>

The following change definitely can't be correct because OFP_VLAN_NONE
is 0xffff:
>  void
>  flow_set_vlan_vid(struct flow *flow, ovs_be16 vid)
>  {
> +    vid &= htons(VLAN_VID_MASK | VLAN_CFI);
>      if (vid == htons(OFP_VLAN_NONE)) {
>          flow->vlan_tci = htons(0);
>      } else {
> -        vid &= htons(VLAN_VID_MASK);
>          flow->vlan_tci &= ~htons(VLAN_VID_MASK);
>          flow->vlan_tci |= htons(VLAN_CFI) | vid;
>      }

(If the unit tests don't catch that then we need to improve the unit
tests.)

Stepping back, I think that there might be some missing context here.
In particular, these functions that you're looking at date back one way
or another to the earliest days of Open vSwitch, where OpenFlow
(pre-)1.0 was the only standard and NXM hadn't been thought of yet.  So
they implicitly work with what OF1.0 expects, such as OFP_VLAN_NONE.
Perhaps that needs to go in a comment or in the function name
(e.g. "flow_set_vlan_vid_of10").



More information about the dev mailing list