Hi Andy,
Two comments inline
Other than those, LGTM, but review by others might be useful :)
Acked-by: Daniele Di Proietto <ddiproie...@vmware.com>
On 8/12/14, 1:45 PM, "Andy Zhou" <az...@nicira.com> wrote:
>-1 was used to indicate both nested and variable length attributes.
>This patch introduces OVS_KEY_LEN_NESTED and OVS_KEY_LEN_VARIABLE to
>tell them apart.
>
>Refactor nlattr_set() to more more generally all ovs netlink key
>attributes.
Typo in the commit message (more more)?
>
>Signed-off-by: Andy Zhou <az...@nicira.com>
>---
> datapath/flow_netlink.c | 88
>++++++++++++++++++++++++++++---------------------
> datapath/flow_netlink.h | 6 ++++
> 2 files changed, 57 insertions(+), 37 deletions(-)
>
>diff --git a/datapath/flow_netlink.c b/datapath/flow_netlink.c
>index e525c9d..cc5636b 100644
>--- a/datapath/flow_netlink.c
>+++ b/datapath/flow_netlink.c
>@@ -248,7 +248,7 @@ static bool match_validate(const struct sw_flow_match
>*match,
>
> /* 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_ENCAP] = OVS_KEY_LEN_NESTED,
> [OVS_KEY_ATTR_PRIORITY] = sizeof(u32),
> [OVS_KEY_ATTR_IN_PORT] = sizeof(u32),
> [OVS_KEY_ATTR_SKB_MARK] = sizeof(u32),
>@@ -267,10 +267,24 @@ static const int ovs_key_lens[OVS_KEY_ATTR_MAX + 1]
>= {
> [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_TUNNEL] = OVS_KEY_LEN_NESTED,
> [OVS_KEY_ATTR_MPLS] = sizeof(struct ovs_key_mpls),
> };
>
>+/* The size of the argument for each %OVS_TUNNEL_KEY_ATTR_* Netlink
>+ * attribute. */
>+static const int 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_OAM] = 0,
>+ [OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS] = OVS_KEY_LEN_VARIABLE,
>+};
>+
> static bool is_all_zero(const u8 *fp, size_t size)
> {
> int i;
>@@ -310,7 +324,8 @@ static int __parse_flow_nlattrs(const struct nlattr
>*attr,
> }
>
> expected_len = ovs_key_lens[type];
>- if (nla_len(nla) != expected_len && expected_len != -1) {
>+ if (nla_len(nla) != expected_len &&
>+ expected_len != OVS_KEY_LEN_NESTED) {
> OVS_NLERR("Key attribute has unexpected length (type=%d"
> ", length=%d, expected=%d).\n", type,
> nla_len(nla), expected_len);
>@@ -353,17 +368,6 @@ static int ipv4_tun_from_nlattr(const struct nlattr
>*attr,
>
> 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_OAM] = 0,
>- [OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS] = -1,
>- };
>
> if (type > OVS_TUNNEL_KEY_ATTR_MAX) {
> OVS_NLERR("Unknown IPv4 tunnel attribute (type=%d,
> max=%d).\n",
>@@ -372,7 +376,7 @@ static int ipv4_tun_from_nlattr(const struct nlattr
>*attr,
> }
>
> if (ovs_tunnel_key_lens[type] != nla_len(a) &&
>- ovs_tunnel_key_lens[type] != -1) {
>+ ovs_tunnel_key_lens[type] != OVS_KEY_LEN_VARIABLE) {
> 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]);
>@@ -819,26 +823,33 @@ static int ovs_key_from_nlattrs(struct
>sw_flow_match *match, u64 attrs,
> return 0;
> }
>
>-static void nlattr_set(struct nlattr *attr, u8 val, bool
>is_attr_mask_key)
>+static void nlattr_set(struct nlattr *attr, u8 val, const int key_lens[])
> {
> struct nlattr *nla;
> int rem;
>
> /* The nlattr stream should already have been validated */
> nla_for_each_nested(nla, attr, rem) {
>- /* We assume that ovs_key_lens[type] == -1 means that type is a
>- * nested attribute
>- */
>- if (is_attr_mask_key && ovs_key_lens[nla_type(nla)] == -1)
>- nlattr_set(nla, val, false);
>- else
>+ int len = key_lens[nla_type(nla)];
>+
>+ if (len > 0 || len != OVS_KEY_LEN_NESTED)
I don¹t think the len > 0 check is necessary.
> memset(nla_data(nla), val, nla_len(nla));
>- }
>-}
>+ else if ((len == OVS_KEY_LEN_NESTED) &&
>+ (key_lens == ovs_key_lens))
>+ switch(nla_type(nla)) {
>+ case OVS_KEY_ATTR_ENCAP:
>+ nlattr_set(attr, val, ovs_key_lens);
>+ return;
>+
>+ case OVS_KEY_ATTR_TUNNEL:
>+ nlattr_set(attr, val,
>+ ovs_tunnel_key_lens);
>+ return;
>
>-static void mask_set_nlattr(struct nlattr *attr, u8 val)
>-{
>- nlattr_set(attr, val, true);
>+ }
>+
>+ BUG_ON(len != 0);
>+ }
> }
>
> /**
>@@ -925,7 +936,7 @@ int ovs_nla_get_match(struct sw_flow_match *match,
> if (!newmask)
> return -ENOMEM;
>
>- mask_set_nlattr(newmask, 0xff);
>+ nlattr_set(newmask, 0xff, ovs_key_lens);
>
> /* The userspace does not send tunnel attributes that
> * are 0, but we should not wildcard them nonetheless.
>@@ -1529,7 +1540,7 @@ static int validate_set(const struct nlattr *a,
>
> if (key_type > OVS_KEY_ATTR_MAX ||
> (ovs_key_lens[key_type] != nla_len(ovs_key) &&
>- ovs_key_lens[key_type] != -1))
>+ ovs_key_lens[key_type] != OVS_KEY_LEN_NESTED))
> return -EINVAL;
>
> switch (key_type) {
>@@ -1661,17 +1672,20 @@ static int __ovs_nla_copy_actions(const struct
>nlattr *attr,
> return -EOVERFLOW;
>
> nla_for_each_nested(a, attr, rem) {
>- /* Expected argument lengths, (u32)-1 for variable length. */
>- static const u32 action_lens[OVS_ACTION_ATTR_MAX + 1] = {
>+ /* Expected argument lengths, or OVS_KEY_LEN_NETSTED for
>+ * nested actions attributes. */
>+ static const int action_lens[OVS_ACTION_ATTR_MAX + 1] = {
> [OVS_ACTION_ATTR_OUTPUT] = sizeof(u32),
> [OVS_ACTION_ATTR_RECIRC] = sizeof(u32),
>- [OVS_ACTION_ATTR_USERSPACE] = (u32)-1,
>- [OVS_ACTION_ATTR_PUSH_MPLS] = sizeof(struct
>ovs_action_push_mpls),
>+ [OVS_ACTION_ATTR_USERSPACE] = OVS_KEY_LEN_NESTED,
>+ [OVS_ACTION_ATTR_PUSH_MPLS] =
>+ sizeof(struct ovs_action_push_mpls),
> [OVS_ACTION_ATTR_POP_MPLS] = sizeof(__be16),
>- [OVS_ACTION_ATTR_PUSH_VLAN] = sizeof(struct
>ovs_action_push_vlan),
>+ [OVS_ACTION_ATTR_PUSH_VLAN] =
>+ sizeof(struct ovs_action_push_vlan),
> [OVS_ACTION_ATTR_POP_VLAN] = 0,
>- [OVS_ACTION_ATTR_SET] = (u32)-1,
>- [OVS_ACTION_ATTR_SAMPLE] = (u32)-1,
>+ [OVS_ACTION_ATTR_SET] = OVS_KEY_LEN_NESTED,
>+ [OVS_ACTION_ATTR_SAMPLE] = OVS_KEY_LEN_NESTED,
> [OVS_ACTION_ATTR_HASH] = sizeof(struct ovs_action_hash)
> };
> const struct ovs_action_push_vlan *vlan;
>@@ -1680,7 +1694,7 @@ static int __ovs_nla_copy_actions(const struct
>nlattr *attr,
>
> if (type > OVS_ACTION_ATTR_MAX ||
> (action_lens[type] != nla_len(a) &&
>- action_lens[type] != (u32)-1))
>+ action_lens[type] != (u32)OVS_KEY_LEN_NESTED))
> return -EINVAL;
>
> skip_copy = false;
>diff --git a/datapath/flow_netlink.h b/datapath/flow_netlink.h
>index 8ac40b5..d8db30b 100644
>--- a/datapath/flow_netlink.h
>+++ b/datapath/flow_netlink.h
>@@ -56,4 +56,10 @@ int ovs_nla_put_actions(const struct nlattr *attr,
>
> void ovs_nla_free_flow_actions(struct sw_flow_actions *);
>
>+enum {
>+ /* values >= 0 are fixed key len */
>+ OVS_KEY_LEN_NESTED = -1,
>+ OVS_KEY_LEN_VARIABLE = -2,
>+};
>+
> #endif /* flow_netlink.h */
>--
>1.9.1
>
>_______________________________________________
>dev mailing list
>dev@openvswitch.org
>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/
>listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjtFIdhBDBaw5z%2BU
>6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=7v2sJ9zJqPKVfcTXveTtTOuq8257Iyw1VlD0pe%2F
>%2FZiE%3D%0A&s=850ba89bb67416a265803193bbcaf2f0223ccbb9a46ee14053cbcb44ade
>47a80
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
