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

netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH iproute2-next 0/2] ip/tunnel: Unify collect metadata handling
@ 2018-01-22 16:46 Serhey Popovych
  2018-01-22 16:46 ` [PATCH iproute2-next 1/2] ip/tunnel: Be consistent when printing tunnel collect metadata Serhey Popovych
  2018-01-22 16:46 ` [PATCH iproute2-next 2/2] gre/gre6: Unify attribute addition to netlink buffer Serhey Popovych
  0 siblings, 2 replies; 5+ messages in thread
From: Serhey Popovych @ 2018-01-22 16:46 UTC (permalink / raw)
  To: netdev; +Cc: jbenc, u9012063, julien

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH iproute2-next 1/2] ip/tunnel: Be consistent when printing tunnel collect metadata
  2018-01-22 16:46 [PATCH iproute2-next 0/2] ip/tunnel: Unify collect metadata handling Serhey Popovych
@ 2018-01-22 16:46 ` Serhey Popovych
  2018-01-22 16:58   ` Jiri Benc
  2018-01-22 16:46 ` [PATCH iproute2-next 2/2] gre/gre6: Unify attribute addition to netlink buffer Serhey Popovych
  1 sibling, 1 reply; 5+ messages in thread
From: Serhey Popovych @ 2018-01-22 16:46 UTC (permalink / raw)
  To: netdev; +Cc: jbenc, u9012063, julien

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH iproute2-next 1/2] ip/tunnel: Be consistent when printing tunnel collect metadata
  2018-01-22 16:46 ` [PATCH iproute2-next 1/2] ip/tunnel: Be consistent when printing tunnel collect metadata Serhey Popovych
@ 2018-01-22 16:58   ` Jiri Benc
  2018-01-22 17:00     ` Serhey Popovych
  0 siblings, 1 reply; 5+ messages in thread
From: Jiri Benc @ 2018-01-22 16:58 UTC (permalink / raw)
  To: Serhey Popovych; +Cc: netdev, u9012063, julien

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH iproute2-next 1/2] ip/tunnel: Be consistent when printing tunnel collect metadata
  2018-01-22 16:58   ` Jiri Benc
@ 2018-01-22 17:00     ` Serhey Popovych
  0 siblings, 0 replies; 5+ messages in thread
From: Serhey Popovych @ 2018-01-22 17:00 UTC (permalink / raw)
  To: Jiri Benc; +Cc: netdev, u9012063, julien


[-- 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 --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH iproute2-next 2/2] gre/gre6: Unify attribute addition to netlink buffer
  2018-01-22 16:46 [PATCH iproute2-next 0/2] ip/tunnel: Unify collect metadata handling Serhey Popovych
  2018-01-22 16:46 ` [PATCH iproute2-next 1/2] ip/tunnel: Be consistent when printing tunnel collect metadata Serhey Popovych
@ 2018-01-22 16:46 ` Serhey Popovych
  1 sibling, 0 replies; 5+ messages in thread
From: Serhey Popovych @ 2018-01-22 16:46 UTC (permalink / raw)
  To: netdev; +Cc: jbenc, u9012063, julien

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-01-22 17:00 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-22 16:46 [PATCH iproute2-next 0/2] ip/tunnel: Unify collect metadata handling Serhey Popovych
2018-01-22 16:46 ` [PATCH iproute2-next 1/2] ip/tunnel: Be consistent when printing tunnel collect metadata Serhey Popovych
2018-01-22 16:58   ` Jiri Benc
2018-01-22 17:00     ` Serhey Popovych
2018-01-22 16:46 ` [PATCH iproute2-next 2/2] gre/gre6: Unify attribute addition to netlink buffer Serhey Popovych

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).