[PATCH iproute2-next v2 0/2] ip/tunnel: Unify collect metadata handling

[PATCH iproute2-next v2 0/2] ip/tunnel: Unify collect metadata handling

[Serhey Popovych]

With this series I want to unify collect metadata
handling in tunnels:

  1) Use "external" name for JSON and non-JSON output.

     Do not *print* any options when tunnel in
     collect metadata mode: gre6 already do
     this, so just apply to others.

  2) Do not *add* any attributes when configuring
     gre tunnel in collect metadata mode.

     Other tunnels (e.g. gre6, iptnl, ip6tnl)
     alredy do that.

This is next step in ipv4 and ipv6 modules
unification to prepare for merge in the future.

Any comments, suggestions and criticism as always
welcome.

v2
  For all tunnels implementing collect metadata
  use "external" keyword for both JSON. Thanks
  to Jiri Benc for detailed explanation.

Thanks,
Serhii

Serhey Popovych (2):
  ip/tunnel: Be consistent when printing tunnel collect metadata
  gre/gre6: Unify attribute addition to netlink buffer

 ip/iplink_geneve.c |    8 +++++---
 ip/iplink_vxlan.c  |   10 ++++++----
 ip/link_gre.c      |   50 ++++++++++++++++++++++++--------------------------
 ip/link_gre6.c     |   15 +++++++++------
 ip/link_ip6tnl.c   |    6 ++++--
 ip/link_iptnl.c    |    6 ++++--
 6 files changed, 52 insertions(+), 43 deletions(-)

-- 
1.7.10.4

[PATCH iproute2-next v2 1/2] ip/tunnel: Be consistent when printing tunnel collect metadata

[Serhey Popovych]

Print only "external" if collect meta data attribute
is given: rest of parameters are irrelevant. This is
to follow gre6.

For both JSON and non-JSON output use "external" for
all tunnels including vxlan and geneve.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/iplink_geneve.c |    8 +++++---
 ip/iplink_vxlan.c  |   10 ++++++----
 ip/link_gre.c      |   27 ++++++++++++---------------
 ip/link_gre6.c     |    2 +-
 ip/link_ip6tnl.c   |    6 ++++--
 ip/link_iptnl.c    |    6 ++++--
 6 files changed, 32 insertions(+), 27 deletions(-)

diff --git a/ip/iplink_geneve.c b/ip/iplink_geneve.c
index 1c28bb9..c666072 100644
--- a/ip/iplink_geneve.c
+++ b/ip/iplink_geneve.c
@@ -230,6 +230,11 @@ static void geneve_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	if (!tb)
 		return;
 
+	if (tb[IFLA_GENEVE_COLLECT_METADATA]) {
+		print_bool(PRINT_ANY, "external", "external", true);
+		return;
+	}
+
 	if (!tb[IFLA_GENEVE_ID] ||
 	    RTA_PAYLOAD(tb[IFLA_GENEVE_ID]) < sizeof(__u32))
 		return;
@@ -292,9 +297,6 @@ static void geneve_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 			   "dstport %u ",
 			   rta_getattr_be16(tb[IFLA_GENEVE_PORT]));
 
-	if (tb[IFLA_GENEVE_COLLECT_METADATA])
-		print_bool(PRINT_ANY, "collect_metadata", "external ", true);
-
 	if (tb[IFLA_GENEVE_UDP_CSUM]) {
 		if (is_json_context()) {
 			print_bool(PRINT_JSON,
diff --git a/ip/iplink_vxlan.c b/ip/iplink_vxlan.c
index 842c356..f8cc471 100644
--- a/ip/iplink_vxlan.c
+++ b/ip/iplink_vxlan.c
@@ -381,6 +381,12 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	if (!tb)
 		return;
 
+	if (tb[IFLA_VXLAN_COLLECT_METADATA] &&
+	    rta_getattr_u8(tb[IFLA_VXLAN_COLLECT_METADATA])) {
+		print_bool(PRINT_ANY, "external", "external", true);
+		return;
+	}
+
 	if (!tb[IFLA_VXLAN_ID] ||
 	    RTA_PAYLOAD(tb[IFLA_VXLAN_ID]) < sizeof(__u32))
 		return;
@@ -582,10 +588,6 @@ static void vxlan_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	    rta_getattr_u8(tb[IFLA_VXLAN_REMCSUM_RX]))
 		print_bool(PRINT_ANY, "remcsum_rx", "remcsumrx ", true);
 
-	if (tb[IFLA_VXLAN_COLLECT_METADATA] &&
-	    rta_getattr_u8(tb[IFLA_VXLAN_COLLECT_METADATA]))
-		print_bool(PRINT_ANY, "collect_metadata", "external ", true);
-
 	if (tb[IFLA_VXLAN_GBP])
 		print_bool(PRINT_ANY, "gbp", "gbp ", true);
 	if (tb[IFLA_VXLAN_GPE])
diff --git a/ip/link_gre.c b/ip/link_gre.c
index 674603b..8eb60d1 100644
--- a/ip/link_gre.c
+++ b/ip/link_gre.c
@@ -395,7 +395,7 @@ get_failed:
 	return 0;
 }
 
-static void gre_print_direct_opt(FILE *f, struct rtattr *tb[])
+static void gre_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 {
 	char s2[64];
 	const char *local = "any";
@@ -405,6 +405,14 @@ static void gre_print_direct_opt(FILE *f, struct rtattr *tb[])
 	__u8 ttl = 0;
 	__u8 tos = 0;
 
+	if (!tb)
+		return;
+
+	if (tb[IFLA_GRE_COLLECT_METADATA]) {
+		print_bool(PRINT_ANY, "external", "external", true);
+		return;
+	}
+
 	if (tb[IFLA_GRE_REMOTE]) {
 		unsigned int addr = rta_getattr_u32(tb[IFLA_GRE_REMOTE]);
 
@@ -455,6 +463,9 @@ static void gre_print_direct_opt(FILE *f, struct rtattr *tb[])
 			print_bool(PRINT_JSON, "pmtudisc", NULL, true);
 	}
 
+	if (tb[IFLA_GRE_IGNORE_DF] && rta_getattr_u8(tb[IFLA_GRE_IGNORE_DF]))
+		print_bool(PRINT_ANY, "ignore_df", "ignore-df ", true);
+
 	if (tb[IFLA_GRE_IFLAGS])
 		iflags = rta_getattr_u16(tb[IFLA_GRE_IFLAGS]);
 
@@ -488,20 +499,6 @@ static void gre_print_direct_opt(FILE *f, struct rtattr *tb[])
 				    "fwmark", "fwmark 0x%x ", fwmark);
 		}
 	}
-}
-
-static void gre_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
-{
-	if (!tb)
-		return;
-
-	if (!tb[IFLA_GRE_COLLECT_METADATA])
-		gre_print_direct_opt(f, tb);
-	else
-		print_bool(PRINT_ANY, "external", "external ", true);
-
-	if (tb[IFLA_GRE_IGNORE_DF] && rta_getattr_u8(tb[IFLA_GRE_IGNORE_DF]))
-		print_bool(PRINT_ANY, "ignore_df", "ignore-df ", true);
 
 	if (tb[IFLA_GRE_ERSPAN_INDEX]) {
 		__u32 erspan_idx = rta_getattr_u32(tb[IFLA_GRE_ERSPAN_INDEX]);
diff --git a/ip/link_gre6.c b/ip/link_gre6.c
index a0eeb5a..60c0b54 100644
--- a/ip/link_gre6.c
+++ b/ip/link_gre6.c
@@ -438,7 +438,7 @@ static void gre_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 		return;
 
 	if (tb[IFLA_GRE_COLLECT_METADATA]) {
-		print_bool(PRINT_ANY, "collect_metadata", "external", true);
+		print_bool(PRINT_ANY, "external", "external", true);
 		return;
 	}
 
diff --git a/ip/link_ip6tnl.c b/ip/link_ip6tnl.c
index 8f5c9bd..3a20fe2 100644
--- a/ip/link_ip6tnl.c
+++ b/ip/link_ip6tnl.c
@@ -341,8 +341,10 @@ static void ip6tunnel_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb
 	if (!tb)
 		return;
 
-	if (tb[IFLA_IPTUN_COLLECT_METADATA])
-		print_bool(PRINT_ANY, "external", "external ", true);
+	if (tb[IFLA_IPTUN_COLLECT_METADATA]) {
+		print_bool(PRINT_ANY, "external", "external", true);
+		return;
+	}
 
 	if (tb[IFLA_IPTUN_FLAGS])
 		flags = rta_getattr_u32(tb[IFLA_IPTUN_FLAGS]);
diff --git a/ip/link_iptnl.c b/ip/link_iptnl.c
index ce3855c..c7cc0a5 100644
--- a/ip/link_iptnl.c
+++ b/ip/link_iptnl.c
@@ -370,8 +370,10 @@ static void iptunnel_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[
 	if (!tb)
 		return;
 
-	if (tb[IFLA_IPTUN_COLLECT_METADATA])
-		print_bool(PRINT_ANY, "external", "external ", true);
+	if (tb[IFLA_IPTUN_COLLECT_METADATA]) {
+		print_bool(PRINT_ANY, "external", "external", true);
+		return;
+	}
 
 	if (tb[IFLA_IPTUN_PROTO]) {
 		switch (rta_getattr_u8(tb[IFLA_IPTUN_PROTO])) {
-- 
1.7.10.4

[PATCH iproute2-next v2 2/2] gre/gre6: Unify attribute addition to netlink buffer

[Serhey Popovych]

There are couple of minor improvements:

  1) Check erspan_ver == 2 in gre6. It still could
     be 1 if erspan_idx is 0.

  2) Add tunnel encapsulation attributes only when
     collect metadata not in effect in gre.

  3) Trivial: address checkpatch issues.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/link_gre.c  |   23 ++++++++++++-----------
 ip/link_gre6.c |   13 ++++++++-----
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/ip/link_gre.c b/ip/link_gre.c
index 8eb60d1..69bb88e 100644
--- a/ip/link_gre.c
+++ b/ip/link_gre.c
@@ -366,6 +366,8 @@ get_failed:
 		addattr_l(n, 1024, IFLA_GRE_LOCAL, &saddr, 4);
 		addattr_l(n, 1024, IFLA_GRE_REMOTE, &daddr, 4);
 		addattr_l(n, 1024, IFLA_GRE_PMTUDISC, &pmtudisc, 1);
+		if (ignore_df)
+			addattr8(n, 1024, IFLA_GRE_IGNORE_DF, ignore_df & 1);
 		if (link)
 			addattr32(n, 1024, IFLA_GRE_LINK, link);
 		addattr_l(n, 1024, IFLA_GRE_TTL, &ttl, 1);
@@ -374,24 +376,23 @@ get_failed:
 		if (erspan_ver) {
 			addattr8(n, 1024, IFLA_GRE_ERSPAN_VER, erspan_ver);
 			if (erspan_ver == 1 && erspan_idx != 0) {
-				addattr32(n, 1024, IFLA_GRE_ERSPAN_INDEX, erspan_idx);
+				addattr32(n, 1024,
+					  IFLA_GRE_ERSPAN_INDEX, erspan_idx);
 			} else if (erspan_ver == 2) {
-				addattr8(n, 1024, IFLA_GRE_ERSPAN_DIR, erspan_dir);
-				addattr16(n, 1024, IFLA_GRE_ERSPAN_HWID, erspan_hwid);
+				addattr8(n, 1024,
+					 IFLA_GRE_ERSPAN_DIR, erspan_dir);
+				addattr16(n, 1024,
+					  IFLA_GRE_ERSPAN_HWID, erspan_hwid);
 			}
 		}
+		addattr16(n, 1024, IFLA_GRE_ENCAP_TYPE, encaptype);
+		addattr16(n, 1024, IFLA_GRE_ENCAP_FLAGS, encapflags);
+		addattr16(n, 1024, IFLA_GRE_ENCAP_SPORT, htons(encapsport));
+		addattr16(n, 1024, IFLA_GRE_ENCAP_DPORT, htons(encapdport));
 	} else {
 		addattr_l(n, 1024, IFLA_GRE_COLLECT_METADATA, NULL, 0);
 	}
 
-	addattr16(n, 1024, IFLA_GRE_ENCAP_TYPE, encaptype);
-	addattr16(n, 1024, IFLA_GRE_ENCAP_FLAGS, encapflags);
-	addattr16(n, 1024, IFLA_GRE_ENCAP_SPORT, htons(encapsport));
-	addattr16(n, 1024, IFLA_GRE_ENCAP_DPORT, htons(encapdport));
-
-	if (ignore_df)
-		addattr8(n, 1024, IFLA_GRE_IGNORE_DF, ignore_df & 1);
-
 	return 0;
 }
 
diff --git a/ip/link_gre6.c b/ip/link_gre6.c
index 60c0b54..3e80625 100644
--- a/ip/link_gre6.c
+++ b/ip/link_gre6.c
@@ -405,10 +405,13 @@ get_failed:
 		if (erspan_ver) {
 			addattr8(n, 1024, IFLA_GRE_ERSPAN_VER, erspan_ver);
 			if (erspan_ver == 1 && erspan_idx != 0) {
-				addattr32(n, 1024, IFLA_GRE_ERSPAN_INDEX, erspan_idx);
-			} else {
-				addattr8(n, 1024, IFLA_GRE_ERSPAN_DIR, erspan_dir);
-				addattr16(n, 1024, IFLA_GRE_ERSPAN_HWID, erspan_hwid);
+				addattr32(n, 1024,
+					  IFLA_GRE_ERSPAN_INDEX, erspan_idx);
+			} else if (erspan_ver == 2) {
+				addattr8(n, 1024,
+					 IFLA_GRE_ERSPAN_DIR, erspan_dir);
+				addattr16(n, 1024,
+					  IFLA_GRE_ERSPAN_HWID, erspan_hwid);
 			}
 		}
 		addattr16(n, 1024, IFLA_GRE_ENCAP_TYPE, encaptype);
@@ -645,7 +648,7 @@ static void gre_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 }
 
 static void gre_print_help(struct link_util *lu, int argc, char **argv,
-	FILE *f)
+			   FILE *f)
 {
 	print_usage(f);
 }
-- 
1.7.10.4

Re: [PATCH iproute2-next v2 0/2] ip/tunnel: Unify collect metadata handling

[David Ahern]

On 1/22/18 10:23 AM, Serhey Popovych wrote:
> With this series I want to unify collect metadata
> handling in tunnels:
> 
>   1) Use "external" name for JSON and non-JSON output.
> 
>      Do not *print* any options when tunnel in
>      collect metadata mode: gre6 already do
>      this, so just apply to others.
> 
>   2) Do not *add* any attributes when configuring
>      gre tunnel in collect metadata mode.
> 
>      Other tunnels (e.g. gre6, iptnl, ip6tnl)
>      alredy do that.
> 
> This is next step in ipv4 and ipv6 modules
> unification to prepare for merge in the future.
> 
> Any comments, suggestions and criticism as always
> welcome.
> 
> v2
>   For all tunnels implementing collect metadata
>   use "external" keyword for both JSON. Thanks
>   to Jiri Benc for detailed explanation.
> 

Series applied to iproute2-next