[ovs-dev] [v2] datapath: Distinguish between nested and variable length attributes

Daniele Di Proietto ddiproietto at vmware.com
Tue Aug 12 22:25:17 UTC 2014


Hi Andy,

Two comments inline

Other than those, LGTM, but review by others might be useful :)

Acked-by: Daniele Di Proietto <ddiproietto at vmware.com>


On 8/12/14, 1:45 PM, "Andy Zhou" <azhou at 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 <azhou at 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 at 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




More information about the dev mailing list