On Wed, Jul 04, 2012 at 05:49:30PM -0700, Ben Pfaff wrote:
> On Thu, Jul 05, 2012 at 09:47:29AM +0900, Simon Horman wrote:
> > 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?
>
> I'm pretty sure it's what ends up getting called in e.g. ovs-ofctl if
> you do something like "ovs-ofctl add-flow br0 dl_vlan=123,actions=drop".
> (But I didn't check, just now, to be sure.)
Thanks, I will check.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
