[ovs-dev] [PATCH 5/5] Use VLAN_VID_SHIFT, even though it is 0, for consistency.

[Ben Pfaff]

On Mon, Feb 15, 2010 at 10:57:20AM -0500, Jesse Gross wrote:
> On Mon, Feb 15, 2010 at 2:05 AM, Justin Pettit <jpettit at nicira.com> wrote:
> 
> > On Feb 13, 2010, at 10:12 AM, Ben Pfaff wrote:
> >
> > > On Fri, Feb 12, 2010 at 10:03:57PM -0800, Justin Pettit wrote:
> > >> On Feb 12, 2010, at 9:15 PM, Jesse Gross wrote:
> > >>
> > >>> On Fri, Feb 12, 2010 at 9:36 PM, Justin Pettit <jpettit at nicira.com>
> > wrote:
> > >>> I was looking at the function dp_netdev_modify_vlan_tci() in
> > >>> dpif-netdev.c.  Doesn't it seem like we should be applying the mask
> > >>> to the passed in value, too?  So, this:
> > >>>
> > >>> We could do that but we validate the actions when the flows are added
> > so they should already be in the correct range.
> > >>
> > >> Yeah.  I still think it would be good to make the function actually
> > >> correct in case it ever gets used elsewhere.  I'll send it out later,
> > >> but I realize it's super low priority.
> > >
> > > I'd rather add a comment explaining that the caller is responsible for
> > > doing validation.
> >
> >
> > Why is that?  I don't ask this to be difficult--I'm curious because you
> > have a good sense about these things.  To me, I would expect a mask argument
> > to not just zero out the area I wish to write but also prevent clobbering
> > bits outside of that.  It's certainly not an expensive sanity-check.  The
> > way it works now, it seems like it would be easy to have a relatively hard
> > to reproduce bug by passing in a bad argument.
> 
> 
> It may not be an expensive operation but to me it just doesn't seem like the
> right thing to do.  It doesn't make sense to do error checking every time
> that we process a packet on an argument that can only change when a flow is
> added.  We do validation when the value changes, doesn't that make the most
> sense?

Thanks Jesse, that's my rationale too.