On Thu, Jul 05, 2012 at 09:27:05AM +0900, Simon Horman wrote:
> 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.
That sounds like a good plan to me.
> With regards to the use of OFP_VLAN_NONE, I was confused between it and
> OFPVID12_NONE.
We should probably rename OFP_VLAN_NONE to OFP10_VLAN_NONE. I think
it's only used in OpenFlow 1.0.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
