On Feb 14, 2013, at 11:07 AM, Ben Pfaff <b...@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 <jpet...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
