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

Jesse Gross jesse at nicira.com
Mon Feb 15 07:57:20 PST 2010


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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://openvswitch.org/pipermail/dev/attachments/20100215/0b15c6c2/attachment.htm>


More information about the dev mailing list