[ovs-dev] [PATCHv2 2/2] datapath: Use nla_parse_strict() for netlink parsing.

Joe Stringer joestringer at nicira.com
Thu Sep 18 01:02:49 UTC 2014


Thanks for the review, comments inline

On 17 September 2014 17:37, Pravin Shelar <pshelar at nicira.com> wrote:

> On Wed, Sep 10, 2014 at 3:16 AM, Joe Stringer <joestringer at nicira.com>
> wrote:
> > Reduce duplicate code by using nla_policy and nla_parse_strict().
> >
> Thanks for simplifying this code. I have couple of comments.
>
> > Signed-off-by: Joe Stringer <joestringer at nicira.com>
> > ---
> > v2: Remove attrs bitmasks from most functions.
> >     Remove key/mask wrappers for parse_nlattrs().
> >     Rebase against flag-based nla_parse_strict().
> > ---
> >  datapath/flow_netlink.c |  420
> ++++++++++++++++++++++-------------------------
> >  1 file changed, 192 insertions(+), 228 deletions(-)
> >
> > diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
> > index 6c74841..2e3e181 100644
> > --- a/datapath/flow_netlink.c
> > +++ b/datapath/flow_netlink.c
> > @@ -111,11 +111,26 @@ static void update_range(struct sw_flow_match
> *match,
> >                                sizeof((match)->key->field));
>     \
> >         } while (0)
> >
> > +static u64 attrs_to_bitmask(const struct nlattr **a, size_t len)
> > +{
> > +       size_t i;
> > +       u64 mask = 0;
> > +
> > +       for (i = 0; i < len; i++)
> > +               if (a[i])
> > +                       mask |= (1ULL << i);
> > +       return mask;
> > +}
> > +
> >  static bool match_validate(const struct sw_flow_match *match,
> > -                          u64 key_attrs, u64 mask_attrs)
> > +                          const struct nlattr **key_attrs,
> > +                          const struct nlattr **mask_attrs)
> >  {
> >         u64 key_expected = 1ULL << OVS_KEY_ATTR_ETHERNET;
> > -       u64 mask_allowed = key_attrs;  /* At most allow all key
> attributes */
> > +       u64 attrs, mask_allowed;
> > +
> > +       /* At most allow all key attributes */
> > +       mask_allowed = attrs_to_bitmask(key_attrs, OVS_KEY_ATTR_MAX);
> >
> >         /* The following mask attributes allowed only if they
> >          * pass the validation tests. */
> > @@ -229,17 +244,19 @@ static bool match_validate(const struct
> sw_flow_match *match,
> >                 }
> >         }
> >
> > -       if ((key_attrs & key_expected) != key_expected) {
> > +       attrs = attrs_to_bitmask(key_attrs, OVS_KEY_ATTR_MAX);
> > +       if ((attrs & key_expected) != attrs) {
> >                 /* Key attributes check failed. */
> >                 OVS_NLERR("Missing expected key attributes
> (key_attrs=%llx, expected=%llx).\n",
> > -                               (unsigned long long)key_attrs, (unsigned
> long long)key_expected);
> > +                               (unsigned long long)attrs, (unsigned
> long long)key_expected);
> >                 return false;
> >         }
> >
> > -       if ((mask_attrs & mask_allowed) != mask_attrs) {
> > +       attrs = attrs_to_bitmask(mask_attrs, OVS_KEY_ATTR_MAX);
> > +       if ((attrs & mask_allowed) != attrs) {
> >                 /* Mask attributes check failed. */
> >                 OVS_NLERR("Contain more than allowed mask fields
> (mask_attrs=%llx, mask_allowed=%llx).\n",
> > -                               (unsigned long long)mask_attrs,
> (unsigned long long)mask_allowed);
> > +                               (unsigned long long)attrs, (unsigned
> long long)mask_allowed);
> >                 return false;
> >         }
> >
> > @@ -287,161 +304,107 @@ size_t ovs_key_attr_size(void)
> >  }
> >
> >  /* The size of the argument for each %OVS_KEY_ATTR_* Netlink
> attribute.  */
> > -static const int ovs_key_lens[OVS_KEY_ATTR_MAX + 1] = {
> > -       [OVS_KEY_ATTR_ENCAP] = -1,
> > -       [OVS_KEY_ATTR_PRIORITY] = sizeof(u32),
> > -       [OVS_KEY_ATTR_IN_PORT] = sizeof(u32),
> > -       [OVS_KEY_ATTR_SKB_MARK] = sizeof(u32),
> > -       [OVS_KEY_ATTR_ETHERNET] = sizeof(struct ovs_key_ethernet),
> > -       [OVS_KEY_ATTR_VLAN] = sizeof(__be16),
> > -       [OVS_KEY_ATTR_ETHERTYPE] = sizeof(__be16),
> > -       [OVS_KEY_ATTR_IPV4] = sizeof(struct ovs_key_ipv4),
> > -       [OVS_KEY_ATTR_IPV6] = sizeof(struct ovs_key_ipv6),
> > -       [OVS_KEY_ATTR_TCP] = sizeof(struct ovs_key_tcp),
> > -       [OVS_KEY_ATTR_TCP_FLAGS] = sizeof(__be16),
> > -       [OVS_KEY_ATTR_UDP] = sizeof(struct ovs_key_udp),
> > -       [OVS_KEY_ATTR_SCTP] = sizeof(struct ovs_key_sctp),
> > -       [OVS_KEY_ATTR_ICMP] = sizeof(struct ovs_key_icmp),
> > -       [OVS_KEY_ATTR_ICMPV6] = sizeof(struct ovs_key_icmpv6),
> > -       [OVS_KEY_ATTR_ARP] = sizeof(struct ovs_key_arp),
> > -       [OVS_KEY_ATTR_ND] = sizeof(struct ovs_key_nd),
> > -       [OVS_KEY_ATTR_DP_HASH] = sizeof(u32),
> > -       [OVS_KEY_ATTR_RECIRC_ID] = sizeof(u32),
> > -       [OVS_KEY_ATTR_TUNNEL] = -1,
> > -       [OVS_KEY_ATTR_MPLS] = sizeof(struct ovs_key_mpls),
> > +static const struct nla_policy ovs_key_policy[OVS_KEY_ATTR_MAX + 1] = {
> > +       [OVS_KEY_ATTR_ENCAP] = { .type = NLA_NESTED },
> > +       [OVS_KEY_ATTR_PRIORITY] = { .len = sizeof(u32) },
> > +       [OVS_KEY_ATTR_IN_PORT] = { .len = sizeof(u32) },
> > +       [OVS_KEY_ATTR_SKB_MARK] = { .len = sizeof(u32) },
> > +       [OVS_KEY_ATTR_ETHERNET] = { .len = sizeof(struct
> ovs_key_ethernet) },
> > +       [OVS_KEY_ATTR_VLAN] = { .len = sizeof(__be16) },
> > +       [OVS_KEY_ATTR_ETHERTYPE] = { .len = sizeof(__be16) },
> > +       [OVS_KEY_ATTR_IPV4] = { .len = sizeof(struct ovs_key_ipv4) },
> > +       [OVS_KEY_ATTR_IPV6] = { .len = sizeof(struct ovs_key_ipv6) },
> > +       [OVS_KEY_ATTR_TCP] = { .len = sizeof(struct ovs_key_tcp) },
> > +       [OVS_KEY_ATTR_TCP_FLAGS] = { .len = sizeof(__be16) },
> > +       [OVS_KEY_ATTR_UDP] = { .len = sizeof(struct ovs_key_udp) },
> > +       [OVS_KEY_ATTR_SCTP] = { .len = sizeof(struct ovs_key_sctp) },
> > +       [OVS_KEY_ATTR_ICMP] = { .len = sizeof(struct ovs_key_icmp) },
> > +       [OVS_KEY_ATTR_ICMPV6] = { .len = sizeof(struct ovs_key_icmpv6) },
> > +       [OVS_KEY_ATTR_ARP] = { .len = sizeof(struct ovs_key_arp) },
> > +       [OVS_KEY_ATTR_ND] = { .len = sizeof(struct ovs_key_nd) },
> > +       [OVS_KEY_ATTR_DP_HASH] = { .len = sizeof(u32) },
> > +       [OVS_KEY_ATTR_RECIRC_ID] = { .len = sizeof(u32) },
> > +       [OVS_KEY_ATTR_TUNNEL] = { .type = NLA_NESTED },
> > +       [OVS_KEY_ATTR_MPLS] = { .len = sizeof(struct ovs_key_mpls) },
> >  };
> >
> > -static bool is_all_zero(const u8 *fp, size_t size)
> > -{
> > -       int i;
> > -
> > -       if (!fp)
> > -               return false;
> > -
> > -       for (i = 0; i < size; i++)
> > -               if (fp[i])
> > -                       return false;
> > -
> > -       return true;
> > -}
> > -
> > -static int __parse_flow_nlattrs(const struct nlattr *attr,
> > -                               const struct nlattr *a[],
> > -                               u64 *attrsp, bool nz)
> > +static int parse_nlattrs(const struct nlattr *attr,
> > +                        const struct nlattr *a[], int maxtype,
> > +                        const struct nla_policy *policy, bool dup, bool
> nz)
> >  {
> > -       const struct nlattr *nla;
> > -       u64 attrs;
> > -       int rem;
> > -
> > -       attrs = *attrsp;
> > -       nla_for_each_nested(nla, attr, rem) {
> > -               u16 type = nla_type(nla);
> > -               int expected_len;
> > -
> > -               if (type > OVS_KEY_ATTR_MAX) {
> > -                       OVS_NLERR("Unknown key attribute (type=%d,
> max=%d).\n",
> > -                                 type, OVS_KEY_ATTR_MAX);
> > -                       return -EINVAL;
> > -               }
> > +       int err;
> > +       u8 flags = NLA_PARSE_F_NOINIT | NLA_PARSE_F_UNKNOWN
> > +                  | NLA_PARSE_F_TRAILING | NLA_PARSE_F_EXACT_LEN;
> >
> > -               if (attrs & (1ULL << type)) {
> > -                       OVS_NLERR("Duplicate key attribute (type
> %d).\n", type);
> > -                       return -EINVAL;
> > -               }
> > +       if (!dup)
> > +               flags |= NLA_PARSE_F_DUPLICATE;
> > +       if (nz)
> > +               flags |= NLA_PARSE_F_NONZERO;
> >
> > -               expected_len = ovs_key_lens[type];
> > -               if (nla_len(nla) != expected_len && expected_len != -1) {
> > -                       OVS_NLERR("Key attribute has unexpected length
> (type=%d"
> > -                                 ", length=%d, expected=%d).\n", type,
> > -                                 nla_len(nla), expected_len);
> > -                       return -EINVAL;
> > -               }
> > -
> > -               if (!nz || !is_all_zero(nla_data(nla), expected_len)) {
> > -                       attrs |= 1ULL << type;
> > -                       a[type] = nla;
> > -               }
> > -       }
> > -       if (rem) {
> > -               OVS_NLERR("Message has %d unknown bytes.\n", rem);
> > -               return -EINVAL;
> > -       }
> > +       err = nla_parse_strict(a, maxtype, nla_data(attr), nla_len(attr),
> > +                              policy, flags);
> > +       if (err)
> > +               return err;
> >
> > -       *attrsp = attrs;
> >         return 0;
> >  }
> >
> > -static int parse_flow_mask_nlattrs(const struct nlattr *attr,
> > -                                  const struct nlattr *a[], u64 *attrsp)
> > -{
> > -       return __parse_flow_nlattrs(attr, a, attrsp, true);
> > -}
> > -
> > -static int parse_flow_nlattrs(const struct nlattr *attr,
> > -                             const struct nlattr *a[], u64 *attrsp)
> > -{
> > -       return __parse_flow_nlattrs(attr, a, attrsp, false);
> > -}
> > -
> >  static int ipv4_tun_from_nlattr(const struct nlattr *attr,
> >                                 struct sw_flow_match *match, bool
> is_mask)
> >  {
> > -       struct nlattr *a;
> > -       int rem;
> > +       static const struct nla_policy
> ovs_tunnel_key_policy[OVS_TUNNEL_KEY_ATTR_MAX + 1] = {
> > +               [OVS_TUNNEL_KEY_ATTR_ID] = { .len = sizeof(u64) },
> > +               [OVS_TUNNEL_KEY_ATTR_IPV4_SRC] = { .len = sizeof(u32) },
> > +               [OVS_TUNNEL_KEY_ATTR_IPV4_DST] = { .len = sizeof(u32) },
> > +               [OVS_TUNNEL_KEY_ATTR_TOS] = { .len = 1 },
> > +               [OVS_TUNNEL_KEY_ATTR_TTL] = { .len = 1 },
> > +               [OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT] = { .len = 0 },
> > +               [OVS_TUNNEL_KEY_ATTR_CSUM] = { .len = 0 },
> > +               [OVS_TUNNEL_KEY_ATTR_TP_SRC] = { .len = sizeof(u16) },
> > +               [OVS_TUNNEL_KEY_ATTR_TP_DST] = { .len = sizeof(u16) },
> > +               [OVS_TUNNEL_KEY_ATTR_OAM] = { .len = 0 },
> > +               [OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS] = { .type = NLA_NESTED
> },
> > +       };
> > +       const struct nlattr *a[OVS_TUNNEL_KEY_ATTR_MAX + 1];
> >         bool ttl = false;
> >         __be16 tun_flags = 0;
> > +       enum ovs_tunnel_key_attr type;
> > +       int err;
> >
> > -       nla_for_each_nested(a, attr, rem) {
> > -               int type = nla_type(a);
> > -               static const u32
> ovs_tunnel_key_lens[OVS_TUNNEL_KEY_ATTR_MAX + 1] = {
> > -                       [OVS_TUNNEL_KEY_ATTR_ID] = sizeof(u64),
> > -                       [OVS_TUNNEL_KEY_ATTR_IPV4_SRC] = sizeof(u32),
> > -                       [OVS_TUNNEL_KEY_ATTR_IPV4_DST] = sizeof(u32),
> > -                       [OVS_TUNNEL_KEY_ATTR_TOS] = 1,
> > -                       [OVS_TUNNEL_KEY_ATTR_TTL] = 1,
> > -                       [OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT] = 0,
> > -                       [OVS_TUNNEL_KEY_ATTR_CSUM] = 0,
> > -                       [OVS_TUNNEL_KEY_ATTR_TP_SRC] = sizeof(u16),
> > -                       [OVS_TUNNEL_KEY_ATTR_TP_DST] = sizeof(u16),
> > -                       [OVS_TUNNEL_KEY_ATTR_OAM] = 0,
> > -                       [OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS] = -1,
> > -               };
> > +       memset(a, 0, sizeof(*a) * (OVS_TUNNEL_KEY_ATTR_MAX + 1));
> > +       err = parse_nlattrs(attr, a, OVS_TUNNEL_KEY_ATTR_MAX,
> > +                           ovs_tunnel_key_policy, true, is_mask);
> > +       if (err)
> > +               return err;
> >
> > -               if (type > OVS_TUNNEL_KEY_ATTR_MAX) {
> > -                       OVS_NLERR("Unknown IPv4 tunnel attribute
> (type=%d, max=%d).\n",
> > -                       type, OVS_TUNNEL_KEY_ATTR_MAX);
> > -                       return -EINVAL;
> > -               }
> > +       for (type = 0; type <= OVS_TUNNEL_KEY_ATTR_MAX; type++) {
> > +               const struct nlattr *nla;
> >
> > -               if (ovs_tunnel_key_lens[type] != nla_len(a) &&
> > -                   ovs_tunnel_key_lens[type] != -1) {
> > -                       OVS_NLERR("IPv4 tunnel attribute type has
> unexpected "
> > -                                 " length (type=%d, length=%d,
> expected=%d).\n",
> > -                                 type, nla_len(a),
> ovs_tunnel_key_lens[type]);
> > -                       return -EINVAL;
> > -               }
> > +               if (!a[type])
> > +                       continue;
> >
> > +               nla = a[type];
> >                 switch (type) {
> >                 case OVS_TUNNEL_KEY_ATTR_ID:
> >                         SW_FLOW_KEY_PUT(match, tun_key.tun_id,
> > -                                       nla_get_be64(a), is_mask);
> > +                                       nla_get_be64(nla), is_mask);
> >                         tun_flags |= TUNNEL_KEY;
> >                         break;
> >                 case OVS_TUNNEL_KEY_ATTR_IPV4_SRC:
> >                         SW_FLOW_KEY_PUT(match, tun_key.ipv4_src,
> > -                                       nla_get_be32(a), is_mask);
> > +                                       nla_get_be32(nla), is_mask);
> >                         break;
> >                 case OVS_TUNNEL_KEY_ATTR_IPV4_DST:
> >                         SW_FLOW_KEY_PUT(match, tun_key.ipv4_dst,
> > -                                       nla_get_be32(a), is_mask);
> > +                                       nla_get_be32(nla), is_mask);
> >                         break;
> >                 case OVS_TUNNEL_KEY_ATTR_TOS:
> >                         SW_FLOW_KEY_PUT(match, tun_key.ipv4_tos,
> > -                                       nla_get_u8(a), is_mask);
> > +                                       nla_get_u8(nla), is_mask);
> >                         break;
> >                 case OVS_TUNNEL_KEY_ATTR_TTL:
> >                         SW_FLOW_KEY_PUT(match, tun_key.ipv4_ttl,
> > -                                       nla_get_u8(a), is_mask);
> > +                                       nla_get_u8(nla), is_mask);
> >                         ttl = true;
> >                         break;
> >                 case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT:
> > @@ -452,29 +415,29 @@ static int ipv4_tun_from_nlattr(const struct
> nlattr *attr,
> >                         break;
> >                 case OVS_TUNNEL_KEY_ATTR_TP_SRC:
> >                         SW_FLOW_KEY_PUT(match, tun_key.tp_src,
> > -                                       nla_get_be16(a), is_mask);
> > +                                       nla_get_be16(nla), is_mask);
> >                         break;
> >                 case OVS_TUNNEL_KEY_ATTR_TP_DST:
> >                         SW_FLOW_KEY_PUT(match, tun_key.tp_dst,
> > -                                       nla_get_be16(a), is_mask);
> > +                                       nla_get_be16(nla), is_mask);
> >                         break;
> >                 case OVS_TUNNEL_KEY_ATTR_OAM:
> >                         tun_flags |= TUNNEL_OAM;
> >                         break;
> >                 case OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS:
> >                         tun_flags |= TUNNEL_OPTIONS_PRESENT;
> > -                       if (nla_len(a) > sizeof(match->key->tun_opts)) {
> > +                       if (nla_len(nla) > sizeof(match->key->tun_opts))
> {
> >                                 OVS_NLERR("Geneve option length exceeds "
> >                                           "maximum size (len %d, max
> %zu).\n",
> > -                                         nla_len(a),
> > +                                         nla_len(nla),
> >                                           sizeof(match->key->tun_opts));
> >                                 return -EINVAL;
> >                         }
> >
> > -                       if (nla_len(a) % 4 != 0) {
> > +                       if (nla_len(nla) % 4 != 0) {
> >                                 OVS_NLERR("Geneve option length is not "
> >                                           "a multiple of 4 (len %d).\n",
> > -                                         nla_len(a));
> > +                                         nla_len(nla));
> >                                 return -EINVAL;
> >                         }
> >
> > @@ -483,7 +446,7 @@ static int ipv4_tun_from_nlattr(const struct nlattr
> *attr,
> >                          * additional options will be silently matched.
> >                          */
> >                         if (!is_mask) {
> > -                               SW_FLOW_KEY_PUT(match, tun_opts_len,
> nla_len(a),
> > +                               SW_FLOW_KEY_PUT(match, tun_opts_len,
> nla_len(nla),
> >                                                 false);
> >                         } else {
> >                                 /* This is somewhat unusual because it
> looks at
> > @@ -496,10 +459,10 @@ static int ipv4_tun_from_nlattr(const struct
> nlattr *attr,
> >                                  * variable length and we won't have the
> >                                  * information later.
> >                                  */
> > -                               if (match->key->tun_opts_len !=
> nla_len(a)) {
> > +                               if (match->key->tun_opts_len !=
> nla_len(nla)) {
> >                                         OVS_NLERR("Geneve option key
> length (%d)"
> >                                            " is different from mask
> length (%d).",
> > -                                          match->key->tun_opts_len,
> nla_len(a));
> > +                                          match->key->tun_opts_len,
> nla_len(nla));
> >                                         return -EINVAL;
> >                                 }
> >
> > @@ -509,8 +472,8 @@ static int ipv4_tun_from_nlattr(const struct nlattr
> *attr,
> >
> >                         SW_FLOW_KEY_MEMCPY_OFFSET(match,
> >                                 (unsigned long)GENEVE_OPTS((struct
> sw_flow_key *)0,
> > -                                                          nla_len(a)),
> > -                               nla_data(a), nla_len(a), is_mask);
> > +                                                          nla_len(nla)),
> > +                               nla_data(nla), nla_len(nla), is_mask);
> >                         break;
> >                 default:
> >                         OVS_NLERR("Unknown IPv4 tunnel attribute
> (%d).\n", type);
> > @@ -520,11 +483,6 @@ static int ipv4_tun_from_nlattr(const struct nlattr
> *attr,
> >
> >         SW_FLOW_KEY_PUT(match, tun_key.tun_flags, tun_flags, is_mask);
> >
> > -       if (rem > 0) {
> > -               OVS_NLERR("IPv4 tunnel attribute has %d unknown
> bytes.\n", rem);
> > -               return -EINVAL;
> > -       }
> > -
> >         if (!is_mask) {
> >                 if (!match->key->tun_key.ipv4_dst) {
> >                         OVS_NLERR("IPv4 tunnel destination address is
> zero.\n");
> > @@ -611,30 +569,31 @@ int ovs_nla_put_egress_tunnel_key(struct sk_buff
> *skb,
> >                                     egress_tun_info->options_len);
> >  }
> >
> > -static int metadata_from_nlattrs(struct sw_flow_match *match,  u64
> *attrs,
> > +static int metadata_from_nlattrs(struct sw_flow_match *match,
> >                                  const struct nlattr **a, bool is_mask)
> >  {
> > -       if (*attrs & (1ULL << OVS_KEY_ATTR_DP_HASH)) {
> > +       if (a[OVS_KEY_ATTR_DP_HASH]) {
> >                 u32 hash_val = nla_get_u32(a[OVS_KEY_ATTR_DP_HASH]);
> >
> >                 SW_FLOW_KEY_PUT(match, ovs_flow_hash, hash_val, is_mask);
> > -               *attrs &= ~(1ULL << OVS_KEY_ATTR_DP_HASH);
> > +               a[OVS_KEY_ATTR_DP_HASH] = NULL;
> >         }
> If you change a[], then match_validate() does not work.
>

I thought that I was misunderstanding something here, now I see that
ovs_key_from_nlattrs() doesn't modify the caller's version of the attrs
bitmask on master.

Would you prefer that the ovs_key_from_nlattrs() function made a local copy
of 'a' and use that here or just convert it to a bitmask and keep
metadata_from_nlattrs() unchanged from master?



> >
> > -       if (*attrs & (1ULL << OVS_KEY_ATTR_RECIRC_ID)) {
> > +       if (a[OVS_KEY_ATTR_RECIRC_ID]) {
> >                 u32 recirc_id = nla_get_u32(a[OVS_KEY_ATTR_RECIRC_ID]);
> >
> >                 SW_FLOW_KEY_PUT(match, recirc_id, recirc_id, is_mask);
> > -               *attrs &= ~(1ULL << OVS_KEY_ATTR_RECIRC_ID);
> > +               a[OVS_KEY_ATTR_RECIRC_ID] = NULL;
> >         }
> >
> > -       if (*attrs & (1ULL << OVS_KEY_ATTR_PRIORITY)) {
> > -               SW_FLOW_KEY_PUT(match, phy.priority,
> > -                         nla_get_u32(a[OVS_KEY_ATTR_PRIORITY]),
> is_mask);
> > -               *attrs &= ~(1ULL << OVS_KEY_ATTR_PRIORITY);
> > +       if (a[OVS_KEY_ATTR_PRIORITY]) {
> > +               u32 priority = nla_get_u32(a[OVS_KEY_ATTR_PRIORITY]);
> > +
> > +               SW_FLOW_KEY_PUT(match, phy.priority, priority, is_mask);
> > +               a[OVS_KEY_ATTR_PRIORITY] = NULL;
> >         }
> >
> > -       if (*attrs & (1ULL << OVS_KEY_ATTR_IN_PORT)) {
> > +       if (a[OVS_KEY_ATTR_IN_PORT]) {
> >                 u32 in_port = nla_get_u32(a[OVS_KEY_ATTR_IN_PORT]);
> >
> >                 if (is_mask) {
> > @@ -646,36 +605,36 @@ static int metadata_from_nlattrs(struct
> sw_flow_match *match,  u64 *attrs,
> >                 }
> >
> >                 SW_FLOW_KEY_PUT(match, phy.in_port, in_port, is_mask);
> > -               *attrs &= ~(1ULL << OVS_KEY_ATTR_IN_PORT);
> > +               a[OVS_KEY_ATTR_IN_PORT] = NULL;
> >         } else if (!is_mask) {
> >                 SW_FLOW_KEY_PUT(match, phy.in_port, DP_MAX_PORTS,
> is_mask);
> >         }
> >
> > -       if (*attrs & (1ULL << OVS_KEY_ATTR_SKB_MARK)) {
> > +       if (a[OVS_KEY_ATTR_SKB_MARK]) {
> >                 uint32_t mark = nla_get_u32(a[OVS_KEY_ATTR_SKB_MARK]);
> >
> >                 SW_FLOW_KEY_PUT(match, phy.skb_mark, mark, is_mask);
> > -               *attrs &= ~(1ULL << OVS_KEY_ATTR_SKB_MARK);
> > +               a[OVS_KEY_ATTR_SKB_MARK] = NULL;
> >         }
> > -       if (*attrs & (1ULL << OVS_KEY_ATTR_TUNNEL)) {
> > +       if (a[OVS_KEY_ATTR_TUNNEL]) {
> >                 if (ipv4_tun_from_nlattr(a[OVS_KEY_ATTR_TUNNEL], match,
> >                                          is_mask))
> >                         return -EINVAL;
> > -               *attrs &= ~(1ULL << OVS_KEY_ATTR_TUNNEL);
> > +               a[OVS_KEY_ATTR_TUNNEL] = NULL;
> >         }
> >         return 0;
> >  }
> >
> > -static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
> > +static int ovs_key_from_nlattrs(struct sw_flow_match *match,
> >                                 const struct nlattr **a, bool is_mask)
> >  {
> > -       int err;
> > +       int i, err;
> >
> > -       err = metadata_from_nlattrs(match, &attrs, a, is_mask);
> > +       err = metadata_from_nlattrs(match, a, is_mask);
> >         if (err)
> >                 return err;
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_ETHERNET)) {
> > +       if (a[OVS_KEY_ATTR_ETHERNET]) {
> >                 const struct ovs_key_ethernet *eth_key;
> >
> >                 eth_key = nla_data(a[OVS_KEY_ATTR_ETHERNET]);
> > @@ -683,10 +642,10 @@ static int ovs_key_from_nlattrs(struct
> sw_flow_match *match, u64 attrs,
> >                                 eth_key->eth_src, ETH_ALEN, is_mask);
> >                 SW_FLOW_KEY_MEMCPY(match, eth.dst,
> >                                 eth_key->eth_dst, ETH_ALEN, is_mask);
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_ETHERNET);
> > +               a[OVS_KEY_ATTR_ETHERNET] = NULL;
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_VLAN)) {
> > +       if (a[OVS_KEY_ATTR_VLAN]) {
> >                 __be16 tci;
> >
> >                 tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
> > @@ -700,10 +659,10 @@ static int ovs_key_from_nlattrs(struct
> sw_flow_match *match, u64 attrs,
> >                 }
> >
> >                 SW_FLOW_KEY_PUT(match, eth.tci, tci, is_mask);
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_VLAN);
> > +               a[OVS_KEY_ATTR_VLAN] = NULL;
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_ETHERTYPE)) {
> > +       if (a[OVS_KEY_ATTR_ETHERTYPE]) {
> >                 __be16 eth_type;
> >
> >                 eth_type = nla_get_be16(a[OVS_KEY_ATTR_ETHERTYPE]);
> > @@ -717,12 +676,12 @@ static int ovs_key_from_nlattrs(struct
> sw_flow_match *match, u64 attrs,
> >                 }
> >
> >                 SW_FLOW_KEY_PUT(match, eth.type, eth_type, is_mask);
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_ETHERTYPE);
> > +               a[OVS_KEY_ATTR_ETHERTYPE] = NULL;
> >         } else if (!is_mask) {
> >                 SW_FLOW_KEY_PUT(match, eth.type, htons(ETH_P_802_2),
> is_mask);
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_IPV4)) {
> > +       if (a[OVS_KEY_ATTR_IPV4]) {
> >                 const struct ovs_key_ipv4 *ipv4_key;
> >
> >                 ipv4_key = nla_data(a[OVS_KEY_ATTR_IPV4]);
> > @@ -743,10 +702,10 @@ static int ovs_key_from_nlattrs(struct
> sw_flow_match *match, u64 attrs,
> >                                 ipv4_key->ipv4_src, is_mask);
> >                 SW_FLOW_KEY_PUT(match, ipv4.addr.dst,
> >                                 ipv4_key->ipv4_dst, is_mask);
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_IPV4);
> > +               a[OVS_KEY_ATTR_IPV4] = NULL;
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_IPV6)) {
> > +       if (a[OVS_KEY_ATTR_IPV6]) {
> >                 const struct ovs_key_ipv6 *ipv6_key;
> >
> >                 ipv6_key = nla_data(a[OVS_KEY_ATTR_IPV6]);
> > @@ -779,10 +738,10 @@ static int ovs_key_from_nlattrs(struct
> sw_flow_match *match, u64 attrs,
> >                                 sizeof(match->key->ipv6.addr.dst),
> >                                 is_mask);
> >
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_IPV6);
> > +               a[OVS_KEY_ATTR_IPV6] = NULL;
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_ARP)) {
> > +       if (a[OVS_KEY_ATTR_ARP]) {
> >                 const struct ovs_key_arp *arp_key;
> >
> >                 arp_key = nla_data(a[OVS_KEY_ATTR_ARP]);
> > @@ -803,54 +762,54 @@ static int ovs_key_from_nlattrs(struct
> sw_flow_match *match, u64 attrs,
> >                 SW_FLOW_KEY_MEMCPY(match, ipv4.arp.tha,
> >                                 arp_key->arp_tha, ETH_ALEN, is_mask);
> >
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_ARP);
> > +               a[OVS_KEY_ATTR_ARP] = NULL;
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_MPLS)) {
> > +       if (a[OVS_KEY_ATTR_MPLS]) {
> >                 const struct ovs_key_mpls *mpls_key;
> >
> >                 mpls_key = nla_data(a[OVS_KEY_ATTR_MPLS]);
> >                 SW_FLOW_KEY_PUT(match, mpls.top_lse,
> >                                 mpls_key->mpls_lse, is_mask);
> >
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_MPLS);
> > +               a[OVS_KEY_ATTR_MPLS] = NULL;
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_TCP)) {
> > +       if (a[OVS_KEY_ATTR_TCP]) {
> >                 const struct ovs_key_tcp *tcp_key;
> >
> >                 tcp_key = nla_data(a[OVS_KEY_ATTR_TCP]);
> >                 SW_FLOW_KEY_PUT(match, tp.src, tcp_key->tcp_src,
> is_mask);
> >                 SW_FLOW_KEY_PUT(match, tp.dst, tcp_key->tcp_dst,
> is_mask);
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_TCP);
> > +               a[OVS_KEY_ATTR_TCP] = NULL;
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_TCP_FLAGS)) {
> > +       if (a[OVS_KEY_ATTR_TCP_FLAGS]) {
> >                 SW_FLOW_KEY_PUT(match, tp.flags,
> >                                 nla_get_be16(a[OVS_KEY_ATTR_TCP_FLAGS]),
> >                                 is_mask);
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_TCP_FLAGS);
> > +               a[OVS_KEY_ATTR_TCP_FLAGS] = NULL;
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_UDP)) {
> > +       if (a[OVS_KEY_ATTR_UDP]) {
> >                 const struct ovs_key_udp *udp_key;
> >
> >                 udp_key = nla_data(a[OVS_KEY_ATTR_UDP]);
> >                 SW_FLOW_KEY_PUT(match, tp.src, udp_key->udp_src,
> is_mask);
> >                 SW_FLOW_KEY_PUT(match, tp.dst, udp_key->udp_dst,
> is_mask);
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_UDP);
> > +               a[OVS_KEY_ATTR_UDP] = NULL;
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_SCTP)) {
> > +       if (a[OVS_KEY_ATTR_SCTP]) {
> >                 const struct ovs_key_sctp *sctp_key;
> >
> >                 sctp_key = nla_data(a[OVS_KEY_ATTR_SCTP]);
> >                 SW_FLOW_KEY_PUT(match, tp.src, sctp_key->sctp_src,
> is_mask);
> >                 SW_FLOW_KEY_PUT(match, tp.dst, sctp_key->sctp_dst,
> is_mask);
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_SCTP);
> > +               a[OVS_KEY_ATTR_SCTP] = NULL;
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_ICMP)) {
> > +       if (a[OVS_KEY_ATTR_ICMP]) {
> >                 const struct ovs_key_icmp *icmp_key;
> >
> >                 icmp_key = nla_data(a[OVS_KEY_ATTR_ICMP]);
> > @@ -858,10 +817,10 @@ static int ovs_key_from_nlattrs(struct
> sw_flow_match *match, u64 attrs,
> >                                 htons(icmp_key->icmp_type), is_mask);
> >                 SW_FLOW_KEY_PUT(match, tp.dst,
> >                                 htons(icmp_key->icmp_code), is_mask);
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_ICMP);
> > +               a[OVS_KEY_ATTR_ICMP] = NULL;
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_ICMPV6)) {
> > +       if (a[OVS_KEY_ATTR_ICMPV6]) {
> >                 const struct ovs_key_icmpv6 *icmpv6_key;
> >
> >                 icmpv6_key = nla_data(a[OVS_KEY_ATTR_ICMPV6]);
> > @@ -869,10 +828,10 @@ static int ovs_key_from_nlattrs(struct
> sw_flow_match *match, u64 attrs,
> >                                 htons(icmpv6_key->icmpv6_type), is_mask);
> >                 SW_FLOW_KEY_PUT(match, tp.dst,
> >                                 htons(icmpv6_key->icmpv6_code), is_mask);
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_ICMPV6);
> > +               a[OVS_KEY_ATTR_ICMPV6] = NULL;
> >         }
> >
> > -       if (attrs & (1ULL << OVS_KEY_ATTR_ND)) {
> > +       if (a[OVS_KEY_ATTR_ND]) {
> >                 const struct ovs_key_nd *nd_key;
> >
> >                 nd_key = nla_data(a[OVS_KEY_ATTR_ND]);
> > @@ -884,13 +843,14 @@ static int ovs_key_from_nlattrs(struct
> sw_flow_match *match, u64 attrs,
> >                         nd_key->nd_sll, ETH_ALEN, is_mask);
> >                 SW_FLOW_KEY_MEMCPY(match, ipv6.nd.tll,
> >                                 nd_key->nd_tll, ETH_ALEN, is_mask);
> > -               attrs &= ~(1ULL << OVS_KEY_ATTR_ND);
> > +               a[OVS_KEY_ATTR_ND] = NULL;
> >         }
> >
> > -       if (attrs != 0) {
> > -               OVS_NLERR("Unknown key attributes (%llx).\n",
> > -                         (unsigned long long)attrs);
> > -               return -EINVAL;
> > +       for (i = 0; i < OVS_KEY_ATTR_MAX; i++) {
> > +               if (a[i]) {
> > +                       OVS_NLERR("Unknown key attribute (%d).\n", i);
> > +                       return -EINVAL;
> > +               }
> >         }
> >
> Do we still need it with NLA_PARSE_F_UNKNOWN flag?
>

Probably not. I'll fix this up.



More information about the dev mailing list