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

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

[Serhey Popovych]

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

  1) Use "collect_metadata" name for JSON and
     "external" keyword for non-JSON printing.

     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.

Thanks,
Serhii

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

 ip/link_gre.c    |   50 ++++++++++++++++++++++++--------------------------
 ip/link_gre6.c   |   13 ++++++++-----
 ip/link_ip6tnl.c |    6 ++++--
 ip/link_iptnl.c  |    6 ++++--
 4 files changed, 40 insertions(+), 35 deletions(-)

-- 
1.7.10.4

[PATCH iproute2-next 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 JSON output use "collect_metadata" for iptnl, ip6tnl and
gre to match with vxlan, geneve and gre6 modules.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/link_gre.c    |   27 ++++++++++++---------------
 ip/link_ip6tnl.c |    6 ++++--
 ip/link_iptnl.c  |    6 ++++--
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/ip/link_gre.c b/ip/link_gre.c
index 674603b..131a087 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, "collect_metadata", "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_ip6tnl.c b/ip/link_ip6tnl.c
index 8f5c9bd..e4db643 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, "collect_metadata", "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..93a1eb4 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, "collect_metadata", "external", true);
+		return;
+	}
 
 	if (tb[IFLA_IPTUN_PROTO]) {
 		switch (rta_getattr_u8(tb[IFLA_IPTUN_PROTO])) {
-- 
1.7.10.4

[PATCH iproute2-next 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 131a087..12c2e22 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 a0eeb5a..875d2dc 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 1/2] ip/tunnel: Be consistent when printing tunnel collect metadata

[Jiri Benc]

On Mon, 22 Jan 2018 18:46:53 +0200, Serhey Popovych wrote:
> +	if (tb[IFLA_GRE_COLLECT_METADATA]) {
> +		print_bool(PRINT_ANY, "collect_metadata", "external", true);
> +		return;
> +	}

Nacked-by: Jiri Benc <jbenc@redhat.com>

Don't ever use "collect_metadata" for anything visible to the user.

collect_metadata is a *horrible* name. It describes the internal
implementation of the lwtunneling in the kernel and provides zero
explanation to the user about what's that feature good for.

The netlink attribute should have never had such name but it's uAPI and
we have to live with it. But there's no reason to expose this to the
user.

Stick with the "external" name. It explains what it is about: instead of
the traffic being controlled by the tunnel internal logic (or tunnel
control plane, if you want), an external logic needs to be attached to
the tunnel in order for the tunneling to work.

 Jiri

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

[Serhey Popovych]

[-- Attachment #1.1: Type: text/plain, Size: 1074 bytes --]

Jiri Benc wrote:
> On Mon, 22 Jan 2018 18:46:53 +0200, Serhey Popovych wrote:
>> +	if (tb[IFLA_GRE_COLLECT_METADATA]) {
>> +		print_bool(PRINT_ANY, "collect_metadata", "external", true);
>> +		return;
>> +	}
> 
> Nacked-by: Jiri Benc <jbenc@redhat.com>
> 
> Don't ever use "collect_metadata" for anything visible to the user.
> 
> collect_metadata is a *horrible* name. It describes the internal
> implementation of the lwtunneling in the kernel and provides zero
> explanation to the user about what's that feature good for.
> 
> The netlink attribute should have never had such name but it's uAPI and
> we have to live with it. But there's no reason to expose this to the
> user.
> 
> Stick with the "external" name. It explains what it is about: instead of
> the traffic being controlled by the tunnel internal logic (or tunnel
> control plane, if you want), an external logic needs to be attached to
> the tunnel in order for the tunneling to work.

Thanks! That's probably what I expect to hear. Will prepare v2 shortly.

> 
>  Jiri
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]