[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openvswitch-dev
Subject:    [ovs-dev] [PATCH 5/5] Use VLAN_VID_SHIFT, even though it is 0, for consistency.
From:       jpettit () nicira ! com (Justin Pettit)
Date:       2010-02-26 6:27:37
Message-ID: D9586BA8-7A62-4DF6-810E-5F9F5E8A3B1D () nicira ! com
[Download RAW message or body]

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.

--Justin


[prev in list] [next in list] [prev in thread] [next in thread]