[PATCH iproute2 0/3] ip/tunnels: Reuse code, vti6 zero endpoint support and minor cleanup

[PATCH iproute2 0/3] ip/tunnels: Reuse code, vti6 zero endpoint support and minor cleanup

[Serhey Popovych]

In this series I present next set of improvements:

  1) Use tnl_parse_key() to avoid code duplication in tunnel
     configuration via netlink code.

  2) Trivial: use IN6ADDR_ANY_INIT instead of open coded
     initialization of local/remote endpoint in ip6tnl code.

  3) Trivial: drop additional checks for zero endpoint
     in vti6 code. This completes and unifies support for
     unconfiguring local/remote endpoint for tunnel.

See individual patch description message for details.

Thanks,
Serhii

Serhey Popovych (3):
  ip/tunnel: Use tnl_parse_key() to parse tunnel key
  link_ip6tnl: Use IN6ADDR_ANY_INIT to initialize local/remote
    endpoints
  link_vti6: Always add local/remote endpoint attributes

 ip/link_gre.c    |   45 +++++----------------------------------------
 ip/link_gre6.c   |   45 +++++----------------------------------------
 ip/link_ip6tnl.c |    4 ++--
 ip/link_vti.c    |   45 +++++----------------------------------------
 ip/link_vti6.c   |   51 +++++++--------------------------------------------
 ip/tunnel.c      |    5 +++--
 6 files changed, 27 insertions(+), 168 deletions(-)

-- 
1.7.10.4

[PATCH iproute2 1/3] ip/tunnel: Use tnl_parse_key() to parse tunnel key

[Serhey Popovych]

It is added with commit a7ed1520ee96 (ip/tunnel:
introduce tnl_parse_key()) to avoid code duplication
in ip6?tunnel.c.

Reuse it for gre/gre6 and vti/vti6 tunnel rtnl
configuration interface with the same purpose
it is used in tunnel ioctl interface in ip6?tunnel.c.

While there change type of key variables from
unsigned integer to __be32 to reflect nature of the
value they store and place error message in
tnl_parse_key() on a single line to make single
call to fprintf().

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/link_gre.c  |   45 +++++----------------------------------------
 ip/link_gre6.c |   45 +++++----------------------------------------
 ip/link_vti.c  |   45 +++++----------------------------------------
 ip/link_vti6.c |   45 +++++----------------------------------------
 ip/tunnel.c    |    5 +++--
 5 files changed, 23 insertions(+), 162 deletions(-)

diff --git a/ip/link_gre.c b/ip/link_gre.c
index 09f1e44..2397920 100644
--- a/ip/link_gre.c
+++ b/ip/link_gre.c
@@ -81,8 +81,8 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
 	struct rtattr *greinfo[IFLA_GRE_MAX + 1];
 	__u16 iflags = 0;
 	__u16 oflags = 0;
-	unsigned int ikey = 0;
-	unsigned int okey = 0;
+	__be32 ikey = 0;
+	__be32 okey = 0;
 	unsigned int saddr = 0;
 	unsigned int daddr = 0;
 	unsigned int link = 0;
@@ -184,53 +184,18 @@ get_failed:
 
 	while (argc > 0) {
 		if (!matches(*argv, "key")) {
-			unsigned int uval;
-
 			NEXT_ARG();
 			iflags |= GRE_KEY;
 			oflags |= GRE_KEY;
-			if (strchr(*argv, '.'))
-				uval = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr,
-						"Invalid value for \"key\": \"%s\"; it should be an unsigned integer\n", *argv);
-					exit(-1);
-				}
-				uval = htonl(uval);
-			}
-
-			ikey = okey = uval;
+			ikey = okey = tnl_parse_key("key", *argv);
 		} else if (!matches(*argv, "ikey")) {
-			unsigned int uval;
-
 			NEXT_ARG();
 			iflags |= GRE_KEY;
-			if (strchr(*argv, '.'))
-				uval = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr, "invalid value for \"ikey\": \"%s\"; it should be an unsigned integer\n", *argv);
-					exit(-1);
-				}
-				uval = htonl(uval);
-			}
-			ikey = uval;
+			ikey = tnl_parse_key("ikey", *argv);
 		} else if (!matches(*argv, "okey")) {
-			unsigned int uval;
-
 			NEXT_ARG();
 			oflags |= GRE_KEY;
-			if (strchr(*argv, '.'))
-				uval = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr, "invalid value for \"okey\": \"%s\"; it should be an unsigned integer\n", *argv);
-					exit(-1);
-				}
-				uval = htonl(uval);
-			}
-			okey = uval;
+			okey = tnl_parse_key("okey", *argv);
 		} else if (!matches(*argv, "seq")) {
 			iflags |= GRE_SEQ;
 			oflags |= GRE_SEQ;
diff --git a/ip/link_gre6.c b/ip/link_gre6.c
index c22fded..7190ada 100644
--- a/ip/link_gre6.c
+++ b/ip/link_gre6.c
@@ -92,8 +92,8 @@ static int gre_parse_opt(struct link_util *lu, int argc, char **argv,
 	struct rtattr *greinfo[IFLA_GRE_MAX + 1];
 	__u16 iflags = 0;
 	__u16 oflags = 0;
-	unsigned int ikey = 0;
-	unsigned int okey = 0;
+	__be32 ikey = 0;
+	__be32 okey = 0;
 	struct in6_addr raddr = IN6ADDR_ANY_INIT;
 	struct in6_addr laddr = IN6ADDR_ANY_INIT;
 	unsigned int link = 0;
@@ -192,53 +192,18 @@ get_failed:
 
 	while (argc > 0) {
 		if (!matches(*argv, "key")) {
-			unsigned int uval;
-
 			NEXT_ARG();
 			iflags |= GRE_KEY;
 			oflags |= GRE_KEY;
-			if (strchr(*argv, '.'))
-				uval = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr,
-						"Invalid value for \"key\"\n");
-					exit(-1);
-				}
-				uval = htonl(uval);
-			}
-
-			ikey = okey = uval;
+			ikey = okey = tnl_parse_key("key", *argv);
 		} else if (!matches(*argv, "ikey")) {
-			unsigned int uval;
-
 			NEXT_ARG();
 			iflags |= GRE_KEY;
-			if (strchr(*argv, '.'))
-				uval = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr, "invalid value of \"ikey\"\n");
-					exit(-1);
-				}
-				uval = htonl(uval);
-			}
-			ikey = uval;
+			ikey = tnl_parse_key("ikey", *argv);
 		} else if (!matches(*argv, "okey")) {
-			unsigned int uval;
-
 			NEXT_ARG();
 			oflags |= GRE_KEY;
-			if (strchr(*argv, '.'))
-				uval = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr, "invalid value of \"okey\"\n");
-					exit(-1);
-				}
-				uval = htonl(uval);
-			}
-			okey = uval;
+			okey = tnl_parse_key("okey", *argv);
 		} else if (!matches(*argv, "seq")) {
 			iflags |= GRE_SEQ;
 			oflags |= GRE_SEQ;
diff --git a/ip/link_vti.c b/ip/link_vti.c
index 05aefa3..6c5469f 100644
--- a/ip/link_vti.c
+++ b/ip/link_vti.c
@@ -64,8 +64,8 @@ static int vti_parse_opt(struct link_util *lu, int argc, char **argv,
 	struct rtattr *tb[IFLA_MAX + 1];
 	struct rtattr *linkinfo[IFLA_INFO_MAX+1];
 	struct rtattr *vtiinfo[IFLA_VTI_MAX + 1];
-	unsigned int ikey = 0;
-	unsigned int okey = 0;
+	__be32 ikey = 0;
+	__be32 okey = 0;
 	unsigned int saddr = 0;
 	unsigned int daddr = 0;
 	unsigned int link = 0;
@@ -122,49 +122,14 @@ get_failed:
 
 	while (argc > 0) {
 		if (!matches(*argv, "key")) {
-			unsigned int uval;
-
 			NEXT_ARG();
-			if (strchr(*argv, '.'))
-				uval = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr,
-						"Invalid value for \"key\": \"%s\"; it should be an unsigned integer\n", *argv);
-					exit(-1);
-				}
-				uval = htonl(uval);
-			}
-
-			ikey = okey = uval;
+			ikey = okey = tnl_parse_key("key", *argv);
 		} else if (!matches(*argv, "ikey")) {
-			unsigned int uval;
-
 			NEXT_ARG();
-			if (strchr(*argv, '.'))
-				uval = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr, "invalid value for \"ikey\": \"%s\"; it should be an unsigned integer\n", *argv);
-					exit(-1);
-				}
-				uval = htonl(uval);
-			}
-			ikey = uval;
+			ikey = tnl_parse_key("ikey", *argv);
 		} else if (!matches(*argv, "okey")) {
-			unsigned int uval;
-
 			NEXT_ARG();
-			if (strchr(*argv, '.'))
-				uval = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr, "invalid value for \"okey\": \"%s\"; it should be an unsigned integer\n", *argv);
-					exit(-1);
-				}
-				uval = htonl(uval);
-			}
-			okey = uval;
+			okey = tnl_parse_key("okey", *argv);
 		} else if (!matches(*argv, "remote")) {
 			NEXT_ARG();
 			daddr = get_addr32(*argv);
diff --git a/ip/link_vti6.c b/ip/link_vti6.c
index 84824a5..f631839 100644
--- a/ip/link_vti6.c
+++ b/ip/link_vti6.c
@@ -61,8 +61,8 @@ static int vti6_parse_opt(struct link_util *lu, int argc, char **argv,
 	struct rtattr *vtiinfo[IFLA_VTI_MAX + 1];
 	struct in6_addr saddr = IN6ADDR_ANY_INIT;
 	struct in6_addr daddr = IN6ADDR_ANY_INIT;
-	unsigned int ikey = 0;
-	unsigned int okey = 0;
+	__be32 ikey = 0;
+	__be32 okey = 0;
 	unsigned int link = 0;
 	__u32 fwmark = 0;
 	int len;
@@ -117,49 +117,14 @@ get_failed:
 
 	while (argc > 0) {
 		if (!matches(*argv, "key")) {
-			unsigned int uval;
-
 			NEXT_ARG();
-			if (strchr(*argv, '.'))
-				uval = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr,
-						"Invalid value for \"key\": \"%s\"; it should be an unsigned integer\n", *argv);
-					exit(-1);
-				}
-				uval = htonl(uval);
-			}
-
-			ikey = okey = uval;
+			ikey = okey = tnl_parse_key("key", *argv);
 		} else if (!matches(*argv, "ikey")) {
-			unsigned int uval;
-
 			NEXT_ARG();
-			if (strchr(*argv, '.'))
-				uval = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr, "invalid value for \"ikey\": \"%s\"; it should be an unsigned integer\n", *argv);
-					exit(-1);
-				}
-				uval = htonl(uval);
-			}
-			ikey = uval;
+			ikey = tnl_parse_key("ikey", *argv);
 		} else if (!matches(*argv, "okey")) {
-			unsigned int uval;
-
 			NEXT_ARG();
-			if (strchr(*argv, '.'))
-				uval = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr, "invalid value for \"okey\": \"%s\"; it should be an unsigned integer\n", *argv);
-					exit(-1);
-				}
-				uval = htonl(uval);
-			}
-			okey = uval;
+			okey = tnl_parse_key("okey", *argv);
 		} else if (!matches(*argv, "remote")) {
 			inet_prefix addr;
 
diff --git a/ip/tunnel.c b/ip/tunnel.c
index d359eb9..f860103 100644
--- a/ip/tunnel.c
+++ b/ip/tunnel.c
@@ -192,8 +192,9 @@ __be32 tnl_parse_key(const char *name, const char *key)
 		return get_addr32(key);
 
 	if (get_unsigned(&uval, key, 0) < 0) {
-		fprintf(stderr, "invalid value for \"%s\": \"%s\";", name, key);
-		fprintf(stderr, " it should be an unsigned integer\n");
+		fprintf(stderr,
+			"invalid value for \"%s\": \"%s\"; it should be an unsigned integer\n",
+			name, key);
 		exit(-1);
 	}
 	return htonl(uval);
-- 
1.7.10.4

[PATCH iproute2 2/3] link_ip6tnl: Use IN6ADDR_ANY_INIT to initialize local/remote endpoints

[Serhey Popovych]

Use specialized helper to initialize endpoint addresses with
zeros instead of open coding this. This unifies initialization
style with other ipv6 tunnel variants (i.e. gre6 and vti6).

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/link_ip6tnl.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ip/link_ip6tnl.c b/ip/link_ip6tnl.c
index 83a4320..f11ddd5 100644
--- a/ip/link_ip6tnl.c
+++ b/ip/link_ip6tnl.c
@@ -88,8 +88,8 @@ static int ip6tunnel_parse_opt(struct link_util *lu, int argc, char **argv,
 	struct rtattr *linkinfo[IFLA_INFO_MAX+1];
 	struct rtattr *iptuninfo[IFLA_IPTUN_MAX + 1];
 	int len;
-	struct in6_addr laddr = {};
-	struct in6_addr raddr = {};
+	struct in6_addr laddr = IN6ADDR_ANY_INIT;
+	struct in6_addr raddr = IN6ADDR_ANY_INIT;
 	__u8 hop_limit = DEFAULT_TNL_HOP_LIMIT;
 	__u8 encap_limit = IPV6_DEFAULT_TNL_ENCAP_LIMIT;
 	__u32 flowinfo = 0;
-- 
1.7.10.4

[PATCH iproute2 3/3] link_vti6: Always add local/remote endpoint attributes

[Serhey Popovych]

All tunnels already support for parsing/adding zero
endpoints and vti6 isn't an exception.

This check was added as part of commit 2a80154fde40
(vti6: fix local/remote any addr handling) and looks
too restrictive as purpose of change is to avoid
endpoint configuration from uninitialized data.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/link_vti6.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/ip/link_vti6.c b/ip/link_vti6.c
index f631839..4136b0e 100644
--- a/ip/link_vti6.c
+++ b/ip/link_vti6.c
@@ -154,10 +154,8 @@ get_failed:
 	addattr32(n, 1024, IFLA_VTI_IKEY, ikey);
 	addattr32(n, 1024, IFLA_VTI_OKEY, okey);
 
-	if (memcmp(&saddr, &in6addr_any, sizeof(in6addr_any)))
-	    addattr_l(n, 1024, IFLA_VTI_LOCAL, &saddr, sizeof(saddr));
-	if (memcmp(&daddr, &in6addr_any, sizeof(in6addr_any)))
-	    addattr_l(n, 1024, IFLA_VTI_REMOTE, &daddr, sizeof(daddr));
+	addattr_l(n, 1024, IFLA_VTI_LOCAL, &saddr, sizeof(saddr));
+	addattr_l(n, 1024, IFLA_VTI_REMOTE, &daddr, sizeof(daddr));
 	addattr32(n, 1024, IFLA_VTI_FWMARK, fwmark);
 	if (link)
 		addattr32(n, 1024, IFLA_VTI_LINK, link);
-- 
1.7.10.4

Re: [PATCH iproute2 0/3] ip/tunnels: Reuse code, vti6 zero endpoint support and minor cleanup

[Stephen Hemminger]

On Mon, 18 Dec 2017 19:48:02 +0200
Serhey Popovych <serhe.popovych@gmail.com> wrote:

> In this series I present next set of improvements:
> 
>   1) Use tnl_parse_key() to avoid code duplication in tunnel
>      configuration via netlink code.
> 
>   2) Trivial: use IN6ADDR_ANY_INIT instead of open coded
>      initialization of local/remote endpoint in ip6tnl code.
> 
>   3) Trivial: drop additional checks for zero endpoint
>      in vti6 code. This completes and unifies support for
>      unconfiguring local/remote endpoint for tunnel.
> 
> See individual patch description message for details.
> 
> Thanks,
> Serhii
> 
> Serhey Popovych (3):
>   ip/tunnel: Use tnl_parse_key() to parse tunnel key
>   link_ip6tnl: Use IN6ADDR_ANY_INIT to initialize local/remote
>     endpoints
>   link_vti6: Always add local/remote endpoint attributes
> 
>  ip/link_gre.c    |   45 +++++----------------------------------------
>  ip/link_gre6.c   |   45 +++++----------------------------------------
>  ip/link_ip6tnl.c |    4 ++--
>  ip/link_vti.c    |   45 +++++----------------------------------------
>  ip/link_vti6.c   |   51 +++++++--------------------------------------------
>  ip/tunnel.c      |    5 +++--
>  6 files changed, 27 insertions(+), 168 deletions(-)
> 

Applied, thanks