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

Justin Pettit jpettit at nicira.com
Thu Feb 25 22:27:37 PST 2010

On Feb 15, 2010, at 7:57 AM, 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?

I don't fully agree, but I'm clearly outvoted.  :-)  I went ahead and pushed a commit to correct a misleading comment within the function and add some clarifying text for callers.


More information about the dev mailing list