[ovs-dev] [PATCH 1/4] ofproto-dpif: Make initial packet value handling generic.

Justin Pettit jpettit at nicira.com
Fri Feb 15 02:56:23 UTC 2013


On Feb 14, 2013, at 11:07 AM, Ben Pfaff <blp at nicira.com> wrote:

> On Wed, Feb 13, 2013 at 03:31:42PM -0800, Justin Pettit wrote:
>> For VLAN splinters, an "initial_tci" value was introduced that is passed
>> around during flow processing to be used later for action translation.
>> This commit switches to passing around a struct so that additional
>> values beyond TCI can be used.  A future commit will use this.
>> 
>> Signed-off-by: Justin Pettit <jpettit at nicira.com>
> 
> It seems reasonable.
> 
> "sparse" complains.  I think the new 'vlan_tci' member of initial_vals
> should be an ovs_be16.

Good point.  Thanks for catching that.

> The comment on 'vlan_tci' here:
> 
>> +/* Initial values of fields of the packet that may be changed during
>> + * flow processing and needed later. */
>> +struct initial_vals {
>> +    /* This value is normally the same as ->facet->flow.vlan_tci.  Only VLAN
>> +     * splinters can cause it to differ.  This value should be removed when
>> +     * the VLAN splinters feature is no longer needed.  */
>> +    uint16_t vlan_tci;
>> +};
> 
> has the annoying property that it sort of dances around what the value
> really is.  How about this for the comment instead:
> 
>    /* This is the value of vlan_tci in the packet as actually received from
>     * dpif.  This is the same as the facet's flow.vlan_tci unless the packet
>     * was received via a VLAN splinter.  In that case, this value is 0
>     * (because the packet as actually received from the dpif had no 802.1Q
>     * tag) but the facet's flow.vlan_tci is set to the VLAN that the splinter
>     * represents.
>     *
>     * This member should be removed when the VLAN splinters feature is no
>     * longer needed.  */
> 
> I think that's about as clear as I can make it.

That's great.  I used it as-is.

--Justin





More information about the dev mailing list