On Wed, Jul 04, 2012 at 05:34:43PM -0700, Ben Pfaff wrote:
> 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.
Actually, I am still somewhat confused.
It seems to me that flow_set_vlan_vid() is only used in conjunction
with the MFF_VLAN_VID field id. But it is unclear to me how that may
be used other than with OXM. Likewise with MFF_VLAN_PCP.
Am I missing something?
> > 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.
Good idea, I will make a patch for that.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
