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

Justin Pettit jpettit at nicira.com
Fri Feb 12 18:36:12 PST 2010


Yowza.  That is kind of ugly.  I'm not sure that the first two would even correct had the shift not been zero, though.  I think all the masks are bitwise-correct, so you normally don't shift them.

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:

	veh->veth_tci |= htons(tci);

should look like this:

	veh->veth_tci |= htons(tci & mask);

And this:

	tmp.veth_tci = htons(tci);

should look like this:

	tmp.veth_tci = htons(tci & mask);

There's a similar issue in modify_vlan_tci() of datapath/actions.c.  If this seems correct, I'm happy to generate a patch set.

--Justin


On Feb 12, 2010, at 2:17 PM, Ben Pfaff wrote:

> Unfortunately it uglifies the code a bit.  I am not sure that it is
> worth it.
> 
> Reported-by: Jesse Gross <jesse at nicira.com>
> ---
> datapath/datapath.c |    3 ++-
> lib/dpif-netdev.c   |    8 +++++---
> 2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 9f415ac..ca0a482 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -775,7 +775,8 @@ static int validate_actions(const struct sw_flow_actions *actions)
> 			break;
> 
> 		case ODPAT_SET_VLAN_VID:
> -			if (a->vlan_vid.vlan_vid & htons(~VLAN_VID_MASK))
> +			if (a->vlan_vid.vlan_vid
> +			    & htons(~(VLAN_VID_MASK >> VLAN_VID_SHIFT)))
> 				return -EINVAL;
> 			break;
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 1bd8112..7675d4b 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -745,7 +745,8 @@ dpif_netdev_validate_actions(const union odp_action *actions, int n_actions,
> 
> 		case ODPAT_SET_VLAN_VID:
>             *mutates = true;
> -			if (a->vlan_vid.vlan_vid & htons(~VLAN_VID_MASK)) {
> +			if (a->vlan_vid.vlan_vid
> +                & htons(~(VLAN_VID_MASK >> VLAN_VID_SHIFT))) {
> 				return EINVAL;
>             }
> 			break;
> @@ -1268,8 +1269,9 @@ dp_netdev_execute_actions(struct dp_netdev *dp,
> 			break;
> 
> 		case ODPAT_SET_VLAN_VID:
> -			dp_netdev_modify_vlan_tci(packet, key, ntohs(a->vlan_vid.vlan_vid),
> -                                      VLAN_VID_MASK);
> +			dp_netdev_modify_vlan_tci(
> +                packet, key, ntohs(a->vlan_vid.vlan_vid) << VLAN_VID_SHIFT,
> +                VLAN_VID_MASK);
>             break;
> 
> 		case ODPAT_SET_VLAN_PCP:
> -- 
> 1.6.6.1
> 
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org





More information about the dev mailing list