[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