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

[Ben Pfaff]

I pushed out the first four out of the five patches.  I'm still sitting
on 5/5 here trying to decide whether it's worth it.  I don't like it
much myself, and Justin sounded negative about it too (although part of
that was just confusion I guess).

Do you have strong feelings about it, Jesse (or anyone else)?

On Fri, Feb 12, 2010 at 06:48:26PM -0500, Jesse Gross wrote:
> This whole set looks good.
> 
> On Fri, Feb 12, 2010 at 5:17 PM, Ben Pfaff <blp at nicira.com> 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
> >