[ovs-dev] [PATCH 2/2] datapath: Fix missing VLAN netlink attribute handling

Andy Zhou azhou at nicira.com
Mon Jul 29 19:44:21 UTC 2013


On Mon, Jul 29, 2013 at 10:32 AM, Jesse Gross <jesse at nicira.com> wrote:

> On Sat, Jul 27, 2013 at 10:27 PM, Andy Zhou <azhou at nicira.com> wrote:
> > diff --git a/datapath/flow.c b/datapath/flow.c
> > index ba775f4..5ec1b69 100644
> > --- a/datapath/flow.c
> > +++ b/datapath/flow.c
> > @@ -224,6 +224,15 @@ static bool ovs_match_validate(const struct
> sw_flow_match *match,
> >                 return false;
> >         }
> >
> > +       if (match->mask &&
> > +               !(match->mask->key.eth.tci & htons(VLAN_TAG_PRESENT))) {
> > +                       OVS_NLERR("VLAN present bit can not be
> wildcarded.\n");
> > +                       /* Simply log error until user the space program
> is
> > +                        * fixed. Then we can switch to return false from
> > +                        * here.
> > +                        */
> > +       }
>
> Can we just fix this? We already force the bit on for the value in
> userspace, it seems like we could do it for the mask at the same time.
>

We could, but I was hopping to return an error from here eventually.

>
> I think we could also simplify it by just removing the !is_mask test
> in the check in ovs_key_from_nlattrs().
>
That would cause the mask value to be set twice, which is fine. But the
logic would seem inconsistent with IN_PORT and look odd to me.  But I don't
mind changing it if you insist.

> @@ -1317,6 +1326,7 @@ static int metadata_from_nlattrs(struct
sw_flow_match *match,  u64 *attrs,
>                 *attrs &= ~(1ULL << OVS_KEY_ATTR_IN_PORT);
>         } else if (!is_mask) {
>                 SW_FLOW_KEY_PUT(match, phy.in_port, DP_MAX_PORTS,
is_mask);
> +               SW_FLOW_KEY_PUT(match, phy.in_port, 0xffff, !is_mask);

Can you put this in a separate patch? All of these
> attribute-not-present corner cases are getting really nasty and I
> think that the vlan issues are actually somewhat separate.
>
O.K. I will remove it from this patch. We do need to review the handling
for this and other missing attributes.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130729/4beb1b3a/attachment-0003.html>


More information about the dev mailing list