[ovs-dev] [PATCH v2 1/2] meta-flow: Implement mf_force_prereqs().

[Ben Pfaff]

On Tue, Oct 15, 2013 at 10:49:41AM -0700, Jarno Rajahalme wrote:
> 
> On Oct 15, 2013, at 10:06 AM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> 
> > 
> > On Oct 15, 2013, at 9:43 AM, Ben Pfaff <blp at nicira.com> wrote:
> > 
> >> On Tue, Oct 15, 2013 at 09:19:51AM -0700, Jarno Rajahalme wrote:
> >>> Sets mask bits for the given field and its prerequisite fields.
> >>> Needed for unwildcarding the proper bits from datapath masks.
> >>> 
> >>> Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
> >> 
> >> I still think that MFP_VLAN_VID doesn't have any mask prereqs,
> >> e.g. instead of this:
> >>   case MFP_ARP:
> >>   case MFP_IPV4:
> >>   case MFP_IPV6:
> >>   case MFP_MPLS:
> >>   case MFP_VLAN_VID:
> >>   case MFP_IP_ANY:
> >>       mask->dl_type = OVS_BE16_MAX;
> >>       /* Fall through. */
> >>   case MFP_NONE:
> >>       break;
> >> this:
> >>   case MFP_ARP:
> >>   case MFP_IPV4:
> >>   case MFP_IPV6:
> >>   case MFP_MPLS:
> >>   case MFP_IP_ANY:
> >>       mask->dl_type = OVS_BE16_MAX;
> >>       /* Fall through. */
> >>   case MFP_VLAN_VID:
> >>   case MFP_NONE:
> >>       break;
> >> 
> >> Otherwise:
> >> Acked-by: Ben Pfaff <blp at nicira.com>
> > 
> > Sorry that I missed that, I was thinking about the ether type dependency on the wire, and did not remember that it is not visible in struct flow.
> > 
> > But now I wonder if the mask's VLAN_CFI bit should be set in this case, as mf_are_prereqs_ok() checks the CFI bit in this case. Thoughts?
> > 
> 
> Update: MFP_VLAN_VID is only ever used for MFF_VLAN_PCP. The
> flow_set_vlan_pcp() always sets the VLAN_CFI bit as well, so setting
> the bit again in case MFP_VLAN_VID is not necessary, but it may be a
> bit confusing not to set it. I guess it would be prudent to set it
> anyway for clarity.

I think I'd be OK either way.  I guess it's harmless, at worst.