On Wed, Jul 04, 2012 at 02:55:52PM -0700, Ben Pfaff wrote:
> 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 <ho...@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").
Yes, I think it would be a good idea for me to take a step back.
I will see about making flow_set_vlan_vid_of10 and flow_set_vlan_vid_of12
functions and see if that works out a bit better.
With regards to the use of OFP_VLAN_NONE, I was confused between it and
OFPVID12_NONE.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
