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

Justin Pettit jpettit at nicira.com
Mon Feb 15 07:05:17 UTC 2010


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.

--Justin






More information about the dev mailing list