[iproute PATCH 00/12] smaller iptunnel and ip6tunnel review

[iproute PATCH 00/12] smaller iptunnel and ip6tunnel review

[Phil Sutter]

In an effort to try and merge iptunnel and ip6tunnel support code, I found a
few things worth changing, mainly by comparing the two files.

Please note that I did not test functionality of all supported tunnel modes,
but since the changes are fairly small and obvious I hopefully didn't introduce
too many bugs.

Phil Sutter (12):
  ip{,6}tunnel: get rid of extraneous whitespace when printing
  ip/tunnel: introduce tnl_parse_key()
  ip{,6}tunnel: unify behaviour if physical device is not found
  iptunnel: use ll_name_to_index() for physical interface lookup
  ip{,6}tunnel: align do_tunnels_list() a bit
  ip6tunnel: print local/remote addresses like iptunnel does
  ip6tunnel: fix coding style: no newline between brace and else
  iptunnel: share common code when setting tunnel mode
  iptunnel: simplify parsing TTL, allow 'hlim' as identifier
  iptunnel: share common code when determining the default interface
    name
  iptunnel: sanitize copying tunnel name
  ip{,6}tunnel: put spaces around non-unary operators

 ip/ip6tunnel.c |  83 +++++++-------------
 ip/iptunnel.c  | 239 ++++++++++++++++++++++-----------------------------------
 ip/tunnel.c    |  15 ++++
 ip/tunnel.h    |   1 +
 4 files changed, 135 insertions(+), 203 deletions(-)

-- 
2.1.2

[iproute PATCH 01/12] ip{,6}tunnel: get rid of extraneous whitespace when printing

[Phil Sutter]

Put whitespace in the beginning of optional parts, not as suffix
anywhere. Also drop double whitespaces in between words.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ip6tunnel.c |  4 ++--
 ip/iptunnel.c  | 16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index 9884efd..07010d3 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -111,9 +111,9 @@ static void print_tunnel(struct ip6_tnl_parm2 *p)
 			printf(" key %u", ntohl(p->i_key));
 		else if ((p->i_flags|p->o_flags)&GRE_KEY) {
 			if (p->i_flags&GRE_KEY)
-				printf(" ikey %u ", ntohl(p->i_key));
+				printf(" ikey %u", ntohl(p->i_key));
 			if (p->o_flags&GRE_KEY)
-				printf(" okey %u ", ntohl(p->o_key));
+				printf(" okey %u", ntohl(p->o_key));
 		}
 
 		if (p->i_flags&GRE_SEQ)
diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 78fa988..36534f2 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -343,7 +343,7 @@ static void print_tunnel(struct ip_tunnel_parm *p)
 	/* Do not use format_host() for local addr,
 	 * symbolic name will not be useful.
 	 */
-	printf("%s: %s/ip  remote %s  local %s ",
+	printf("%s: %s/ip remote %s local %s",
 	       p->name,
 	       tnl_strproto(p->iph.protocol),
 	       p->iph.daddr ? format_host(AF_INET, 4, &p->iph.daddr, s1, sizeof(s1)) : "any",
@@ -371,13 +371,13 @@ static void print_tunnel(struct ip_tunnel_parm *p)
 	if (p->link) {
 		const char *n = ll_index_to_name(p->link);
 		if (n)
-			printf(" dev %s ", n);
+			printf(" dev %s", n);
 	}
 
 	if (p->iph.ttl)
-		printf(" ttl %d ", p->iph.ttl);
+		printf(" ttl %d", p->iph.ttl);
 	else
-		printf(" ttl inherit ");
+		printf(" ttl inherit");
 
 	if (p->iph.tos) {
 		SPRINT_BUF(b1);
@@ -393,11 +393,11 @@ static void print_tunnel(struct ip_tunnel_parm *p)
 		printf(" nopmtudisc");
 
 	if (p->iph.protocol == IPPROTO_IPV6 && !tnl_ioctl_get_6rd(p->name, &ip6rd) && ip6rd.prefixlen) {
-		printf(" 6rd-prefix %s/%u ",
+		printf(" 6rd-prefix %s/%u",
 		       inet_ntop(AF_INET6, &ip6rd.prefix, s1, sizeof(s1)),
 		       ip6rd.prefixlen);
 		if (ip6rd.relay_prefix) {
-			printf("6rd-relay_prefix %s/%u ",
+			printf(" 6rd-relay_prefix %s/%u",
 			       format_host(AF_INET, 4, &ip6rd.relay_prefix, s1, sizeof(s1)),
 			       ip6rd.relay_prefixlen);
 		}
@@ -407,9 +407,9 @@ static void print_tunnel(struct ip_tunnel_parm *p)
 		printf(" key %u", ntohl(p->i_key));
 	else if ((p->i_flags|p->o_flags)&GRE_KEY) {
 		if (p->i_flags&GRE_KEY)
-			printf(" ikey %u ", ntohl(p->i_key));
+			printf(" ikey %u", ntohl(p->i_key));
 		if (p->o_flags&GRE_KEY)
-			printf(" okey %u ", ntohl(p->o_key));
+			printf(" okey %u", ntohl(p->o_key));
 	}
 
 	if (p->i_flags&GRE_SEQ)
-- 
2.1.2

[iproute PATCH 02/12] ip/tunnel: introduce tnl_parse_key()

[Phil Sutter]

Instead of duplicating the same code six times (key, ikey and okey in
iptunnel and ip6tunnel), have a common parsing routine. This has the
added benefit of having the same verbose error message in ip6tunnel as
well as iptunnel.

I'm not sure if parsing an IPv4 address as key makes sense for
ip6tunnel, but the code was there before so this patch at least doesn't
make it worse.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ip6tunnel.c | 33 +++------------------------------
 ip/iptunnel.c  | 33 +++------------------------------
 ip/tunnel.c    | 15 +++++++++++++++
 ip/tunnel.h    |  1 +
 4 files changed, 22 insertions(+), 60 deletions(-)

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index 07010d3..8b842b6 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -230,45 +230,18 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
 				invarg("not inherit", *argv);
 			p->flags |= IP6_TNL_F_RCV_DSCP_COPY;
 		} else if (strcmp(*argv, "key") == 0) {
-			unsigned uval;
 			NEXT_ARG();
 			p->i_flags |= GRE_KEY;
 			p->o_flags |= GRE_KEY;
-			if (strchr(*argv, '.'))
-				p->i_key = p->o_key = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0) < 0) {
-					fprintf(stderr, "invalid value of \"key\"\n");
-					exit(-1);
-				}
-				p->i_key = p->o_key = htonl(uval);
-			}
+			p->i_key = p->o_key = tnl_parse_key("key", *argv);
 		} else if (strcmp(*argv, "ikey") == 0) {
-			unsigned uval;
 			NEXT_ARG();
 			p->i_flags |= GRE_KEY;
-			if (strchr(*argv, '.'))
-				p->i_key = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0)<0) {
-					fprintf(stderr, "invalid value of \"ikey\"\n");
-					exit(-1);
-				}
-				p->i_key = htonl(uval);
-			}
+			p->i_key = tnl_parse_key("ikey", *argv);
 		} else if (strcmp(*argv, "okey") == 0) {
-			unsigned uval;
 			NEXT_ARG();
 			p->o_flags |= GRE_KEY;
-			if (strchr(*argv, '.'))
-				p->o_key = get_addr32(*argv);
-			else {
-				if (get_unsigned(&uval, *argv, 0)<0) {
-					fprintf(stderr, "invalid value of \"okey\"\n");
-					exit(-1);
-				}
-				p->o_key = htonl(uval);
-			}
+			p->o_key = tnl_parse_key("okey", *argv);
 		} else if (strcmp(*argv, "seq") == 0) {
 			p->i_flags |= GRE_SEQ;
 			p->o_flags |= GRE_SEQ;
diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 36534f2..9c9dc54 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -106,45 +106,18 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 				exit(-1);
 			}
 		} else if (strcmp(*argv, "key") == 0) {
-			unsigned uval;
 			NEXT_ARG();
 			p->i_flags |= GRE_KEY;
 			p->o_flags |= GRE_KEY;
-			if (strchr(*argv, '.'))
-				p->i_key = p->o_key = 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);
-				}
-				p->i_key = p->o_key = htonl(uval);
-			}
+			p->i_key = p->o_key = tnl_parse_key("key", *argv);
 		} else if (strcmp(*argv, "ikey") == 0) {
-			unsigned uval;
 			NEXT_ARG();
 			p->i_flags |= GRE_KEY;
-			if (strchr(*argv, '.'))
-				p->i_key = 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);
-				}
-				p->i_key = htonl(uval);
-			}
+			p->i_key = tnl_parse_key("ikey", *argv);
 		} else if (strcmp(*argv, "okey") == 0) {
-			unsigned uval;
 			NEXT_ARG();
 			p->o_flags |= GRE_KEY;
-			if (strchr(*argv, '.'))
-				p->o_key = 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);
-				}
-				p->o_key = htonl(uval);
-			}
+			p->o_key = tnl_parse_key("okey", *argv);
 		} else if (strcmp(*argv, "seq") == 0) {
 			p->i_flags |= GRE_SEQ;
 			p->o_flags |= GRE_SEQ;
diff --git a/ip/tunnel.c b/ip/tunnel.c
index d69fe84..79f2201 100644
--- a/ip/tunnel.c
+++ b/ip/tunnel.c
@@ -180,3 +180,18 @@ int tnl_ioctl_get_6rd(const char *name, void *p)
 {
 	return tnl_gen_ioctl(SIOCGET6RD, name, p, EINVAL);
 }
+
+__be32 tnl_parse_key(const char *name, const char *key)
+{
+	unsigned uval;
+
+	if (strchr(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");
+		exit(-1);
+	}
+	return htonl(uval);
+}
diff --git a/ip/tunnel.h b/ip/tunnel.h
index 9c2f5d2..9fb4a18 100644
--- a/ip/tunnel.h
+++ b/ip/tunnel.h
@@ -31,5 +31,6 @@ int tnl_del_ioctl(const char *basedev, const char *name, void *p);
 int tnl_prl_ioctl(int cmd, const char *name, void *p);
 int tnl_6rd_ioctl(int cmd, const char *name, void *p);
 int tnl_ioctl_get_6rd(const char *name, void *p);
+__be32 tnl_parse_key(const char *name, const char *key);
 
 #endif
-- 
2.1.2

[iproute PATCH 03/12] ip{,6}tunnel: unify behaviour if physical device is not found

[Phil Sutter]

Make ip6tunnel print an error message as well. While there, get rid of
unnecessary line breaking.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ip6tunnel.c | 4 +++-
 ip/iptunnel.c  | 3 +--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index 8b842b6..410276f 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -278,8 +278,10 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
 	}
 	if (medium[0]) {
 		p->link = ll_name_to_index(medium);
-		if (p->link == 0)
+		if (p->link == 0) {
+			fprintf(stderr, "Cannot find device \"%s\"\n", medium);
 			return -1;
+		}
 	}
 	return 0;
 }
diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 9c9dc54..803bb83 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -228,8 +228,7 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 	if (medium[0]) {
 		p->link = if_nametoindex(medium);
 		if (p->link == 0) {
-			fprintf(stderr, "Cannot find device \"%s\"\n",
-				medium);
+			fprintf(stderr, "Cannot find device \"%s\"\n", medium);
 			return -1;
 		}
 	}
-- 
2.1.2

[iproute PATCH 04/12] iptunnel: use ll_name_to_index() for physical interface lookup

[Phil Sutter]

Although the cache is only initialized in do_show(), this way it is at
least consistent with ip6tunnel.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iptunnel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 803bb83..a547852 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -226,7 +226,7 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 	}
 
 	if (medium[0]) {
-		p->link = if_nametoindex(medium);
+		p->link = ll_name_to_index(medium);
 		if (p->link == 0) {
 			fprintf(stderr, "Cannot find device \"%s\"\n", medium);
 			return -1;
-- 
2.1.2

[iproute PATCH 05/12] ip{,6}tunnel: align do_tunnels_list() a bit

[Phil Sutter]

In iptunnel, declare loop variables inside the loop as done in
ip6tunnel.

Fix and simplify goto logic in ip6tunnel:
- Failure to read over header lines would have left fp opened.
- By returning directly upon fopen() failure, fp can be closed
  unconditionally in the end.

Use the same goto logic in iptunnel, as well.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ip6tunnel.c |  8 +++-----
 ip/iptunnel.c  | 25 +++++++++++++------------
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index 410276f..ba92518 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -326,14 +326,14 @@ static int do_tunnels_list(struct ip6_tnl_parm2 *p)
 	FILE *fp = fopen("/proc/net/dev", "r");
 	if (fp == NULL) {
 		perror("fopen");
-		goto end;
+		return -1;
 	}
 
 	/* skip two lines at the begenning of the file */
 	if (!fgets(buf, sizeof(buf), fp) ||
 	    !fgets(buf, sizeof(buf), fp)) {
 		fprintf(stderr, "/proc/net/dev read error\n");
-		return -1;
+		goto end;
 	}
 
 	while (fgets(buf, sizeof(buf), fp) != NULL) {
@@ -395,10 +395,8 @@ static int do_tunnels_list(struct ip6_tnl_parm2 *p)
 		printf("\n");
 	}
 	err = 0;
-
  end:
-	if (fp)
-		fclose(fp);
+	fclose(fp);
 	return err;
 }
 
diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index a547852..e323c1f 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -396,14 +396,8 @@ static void print_tunnel(struct ip_tunnel_parm *p)
 
 static int do_tunnels_list(struct ip_tunnel_parm *p)
 {
-	char name[IFNAMSIZ];
-	unsigned long  rx_bytes, rx_packets, rx_errs, rx_drops,
-	rx_fifo, rx_frame,
-	tx_bytes, tx_packets, tx_errs, tx_drops,
-	tx_fifo, tx_colls, tx_carrier, rx_multi;
-	struct ip_tunnel_parm p1;
-
 	char buf[512];
+	int err = -1;
 	FILE *fp = fopen("/proc/net/dev", "r");
 	if (fp == NULL) {
 		perror("fopen");
@@ -414,19 +408,24 @@ static int do_tunnels_list(struct ip_tunnel_parm *p)
 	if (!fgets(buf, sizeof(buf), fp) ||
 	    !fgets(buf, sizeof(buf), fp)) {
 		fprintf(stderr, "/proc/net/dev read error\n");
-		fclose(fp);
-		return -1;
+		goto end;
 	}
 
 	while (fgets(buf, sizeof(buf), fp) != NULL) {
+		char name[IFNAMSIZ];
 		int index, type;
+		unsigned long rx_bytes, rx_packets, rx_errs, rx_drops,
+			rx_fifo, rx_frame,
+			tx_bytes, tx_packets, tx_errs, tx_drops,
+			tx_fifo, tx_colls, tx_carrier, rx_multi;
+		struct ip_tunnel_parm p1;
 		char *ptr;
+
 		buf[sizeof(buf) - 1] = 0;
 		if ((ptr = strchr(buf, ':')) == NULL ||
 		    (*ptr++ = 0, sscanf(buf, "%s", name) != 1)) {
 			fprintf(stderr, "Wrong format for /proc/net/dev. Giving up.\n");
-			fclose(fp);
-			return -1;
+			goto end;
 		}
 		if (sscanf(ptr, "%ld%ld%ld%ld%ld%ld%ld%*d%ld%ld%ld%ld%ld%ld%ld",
 			   &rx_bytes, &rx_packets, &rx_errs, &rx_drops,
@@ -467,8 +466,10 @@ static int do_tunnels_list(struct ip_tunnel_parm *p)
 		}
 		printf("\n");
 	}
+	err = 0;
+ end:
 	fclose(fp);
-	return 0;
+	return err;
 }
 
 static int do_show(int argc, char **argv)
-- 
2.1.2

[iproute PATCH 06/12] ip6tunnel: print local/remote addresses like iptunnel does

[Phil Sutter]

This makes output consistent with iptunnel, also supporting reverse DNS
lookup for remote address if requested.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ip6tunnel.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index ba92518..9eb5b2f 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -68,14 +68,17 @@ static void usage(void)
 
 static void print_tunnel(struct ip6_tnl_parm2 *p)
 {
-	char remote[64];
-	char local[64];
-
-	inet_ntop(AF_INET6, &p->raddr, remote, sizeof(remote));
-	inet_ntop(AF_INET6, &p->laddr, local, sizeof(local));
+	char s1[1024];
+	char s2[1024];
 
+	/* Do not use format_host() for local addr,
+	 * symbolic name will not be useful.
+	 */
 	printf("%s: %s/ipv6 remote %s local %s",
-	       p->name, tnl_strproto(p->proto), remote, local);
+	       p->name,
+	       tnl_strproto(p->proto),
+	       format_host(AF_INET6, 16, &p->raddr, s1, sizeof(s1)),
+	       rt_addr_n2a(AF_INET6, 16, &p->laddr, s2, sizeof(s2)));
 	if (p->link) {
 		const char *n = ll_index_to_name(p->link);
 		if (n)
-- 
2.1.2

[iproute PATCH 07/12] ip6tunnel: fix coding style: no newline between brace and else

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ip6tunnel.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index 9eb5b2f..d8957f0 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -262,8 +262,7 @@ static int parse_args(int argc, char **argv, int cmd, struct ip6_tnl_parm2 *p)
 		} else {
 			if (strcmp(*argv, "name") == 0) {
 				NEXT_ARG();
-			}
-			else if (matches(*argv, "help") == 0)
+			} else if (matches(*argv, "help") == 0)
 				usage();
 			if (p->name[0])
 				duparg2("name", *argv);
-- 
2.1.2

[iproute PATCH 08/12] iptunnel: share common code when setting tunnel mode

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iptunnel.c | 39 ++++++++++++++-------------------------
 1 file changed, 14 insertions(+), 25 deletions(-)

diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index e323c1f..92edb34 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -47,6 +47,15 @@ static void usage(void)
 	exit(-1);
 }
 
+static void set_tunnel_proto(struct ip_tunnel_parm *p, int proto)
+{
+	if (p->iph.protocol && p->iph.protocol != proto) {
+		fprintf(stderr,"You managed to ask for more than one tunnel mode.\n");
+		exit(-1);
+	}
+	p->iph.protocol = proto;
+}
+
 static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 {
 	int count = 0;
@@ -68,38 +77,18 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 			NEXT_ARG();
 			if (strcmp(*argv, "ipip") == 0 ||
 			    strcmp(*argv, "ip/ip") == 0) {
-				if (p->iph.protocol && p->iph.protocol != IPPROTO_IPIP) {
-					fprintf(stderr,"You managed to ask for more than one tunnel mode.\n");
-					exit(-1);
-				}
-				p->iph.protocol = IPPROTO_IPIP;
+				set_tunnel_proto(p, IPPROTO_IPIP);
 			} else if (strcmp(*argv, "gre") == 0 ||
 				   strcmp(*argv, "gre/ip") == 0) {
-				if (p->iph.protocol && p->iph.protocol != IPPROTO_GRE) {
-					fprintf(stderr,"You managed to ask for more than one tunnel mode.\n");
-					exit(-1);
-				}
-				p->iph.protocol = IPPROTO_GRE;
+				set_tunnel_proto(p, IPPROTO_GRE);
 			} else if (strcmp(*argv, "sit") == 0 ||
 				   strcmp(*argv, "ipv6/ip") == 0) {
-				if (p->iph.protocol && p->iph.protocol != IPPROTO_IPV6) {
-					fprintf(stderr,"You managed to ask for more than one tunnel mode.\n");
-					exit(-1);
-				}
-				p->iph.protocol = IPPROTO_IPV6;
+				set_tunnel_proto(p, IPPROTO_IPV6);
 			} else if (strcmp(*argv, "isatap") == 0) {
-				if (p->iph.protocol && p->iph.protocol != IPPROTO_IPV6) {
-					fprintf(stderr, "You managed to ask for more than one tunnel mode.\n");
-					exit(-1);
-				}
-				p->iph.protocol = IPPROTO_IPV6;
+				set_tunnel_proto(p, IPPROTO_IPV6);
 				isatap++;
 			} else if (strcmp(*argv, "vti") == 0) {
-				if (p->iph.protocol && p->iph.protocol != IPPROTO_IPIP) {
-					fprintf(stderr, "You managed to ask for more than one tunnel mode.\n");
-					exit(-1);
-				}
-				p->iph.protocol = IPPROTO_IPIP;
+				set_tunnel_proto(p, IPPROTO_IPIP);
 				p->i_flags |= VTI_ISVTI;
 			} else {
 				fprintf(stderr,"Unknown tunnel mode \"%s\"\n", *argv);
-- 
2.1.2

[iproute PATCH 09/12] iptunnel: simplify parsing TTL, allow 'hlim' as identifier

[Phil Sutter]

Instead of parsing an unsigned integer and checking boundaries, simply
parse u8. This and the added ttl alias 'hlim' provide consistency with
ip6tunnel.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iptunnel.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 92edb34..8c05f6f 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -141,14 +141,13 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 			NEXT_ARG();
 			strncpy(medium, *argv, IFNAMSIZ-1);
 		} else if (strcmp(*argv, "ttl") == 0 ||
-			   strcmp(*argv, "hoplimit") == 0) {
-			unsigned uval;
+			   strcmp(*argv, "hoplimit") == 0 ||
+			   strcmp(*argv, "hlim") == 0) {
+			__u8 uval;
 			NEXT_ARG();
 			if (strcmp(*argv, "inherit") != 0) {
-				if (get_unsigned(&uval, *argv, 0))
+				if (get_u8(&uval, *argv, 0))
 					invarg("invalid TTL\n", *argv);
-				if (uval > 255)
-					invarg("TTL must be <=255\n", *argv);
 				p->iph.ttl = uval;
 			}
 		} else if (strcmp(*argv, "tos") == 0 ||
-- 
2.1.2

[iproute PATCH 10/12] iptunnel: share common code when determining the default interface name

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iptunnel.c | 70 +++++++++++++++++++++--------------------------------------
 1 file changed, 25 insertions(+), 45 deletions(-)

diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 8c05f6f..3b46a15 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -239,10 +239,26 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 	return 0;
 }
 
+static const char *tnl_defname(const struct ip_tunnel_parm *p)
+{
+	switch (p->iph.protocol) {
+	case IPPROTO_IPIP:
+		if (p->i_flags & VTI_ISVTI)
+			return "ip_vti0";
+		else
+			return "tunl0";
+	case IPPROTO_GRE:
+		return "gre0";
+	case IPPROTO_IPV6:
+		return "sit0";
+	}
+	return NULL;
+}
 
 static int do_add(int cmd, int argc, char **argv)
 {
 	struct ip_tunnel_parm p;
+	const char *basedev;
 
 	if (parse_args(argc, argv, cmd, &p) < 0)
 		return -1;
@@ -252,21 +268,12 @@ static int do_add(int cmd, int argc, char **argv)
 		return -1;
 	}
 
-	switch (p.iph.protocol) {
-	case IPPROTO_IPIP:
-		if (p.i_flags & VTI_ISVTI)
-			return tnl_add_ioctl(cmd, "ip_vti0", p.name, &p);
-		else
-			return tnl_add_ioctl(cmd, "tunl0", p.name, &p);
-	case IPPROTO_GRE:
-		return tnl_add_ioctl(cmd, "gre0", p.name, &p);
-	case IPPROTO_IPV6:
-		return tnl_add_ioctl(cmd, "sit0", p.name, &p);
-	default:
+	if (!(basedev = tnl_defname(&p))) {
 		fprintf(stderr, "cannot determine tunnel mode (ipip, gre, vti or sit)\n");
 		return -1;
 	}
-	return -1;
+
+	return tnl_add_ioctl(cmd, basedev, p.name, &p);
 }
 
 static int do_del(int argc, char **argv)
@@ -276,20 +283,7 @@ static int do_del(int argc, char **argv)
 	if (parse_args(argc, argv, SIOCDELTUNNEL, &p) < 0)
 		return -1;
 
-	switch (p.iph.protocol) {
-	case IPPROTO_IPIP:
-		if (p.i_flags & VTI_ISVTI)
-			return tnl_del_ioctl("ip_vti0", p.name, &p);
-		else
-			return tnl_del_ioctl("tunl0", p.name, &p);
-	case IPPROTO_GRE:
-		return tnl_del_ioctl("gre0", p.name, &p);
-	case IPPROTO_IPV6:
-		return tnl_del_ioctl("sit0", p.name, &p);
-	default:
-		return tnl_del_ioctl(p.name, p.name, &p);
-	}
-	return -1;
+	return tnl_del_ioctl(tnl_defname(&p) ? : p.name, p.name, &p);
 }
 
 static void print_tunnel(struct ip_tunnel_parm *p)
@@ -462,31 +456,17 @@ static int do_tunnels_list(struct ip_tunnel_parm *p)
 
 static int do_show(int argc, char **argv)
 {
-	int err;
 	struct ip_tunnel_parm p;
+	const char *basedev;
 
 	ll_init_map(&rth);
 	if (parse_args(argc, argv, SIOCGETTUNNEL, &p) < 0)
 		return -1;
 
-	switch (p.iph.protocol) {
-	case IPPROTO_IPIP:
-		if (p.i_flags & VTI_ISVTI)
-			err = tnl_get_ioctl(p.name[0] ? p.name : "ip_vti0", &p);
-		else
-			err = tnl_get_ioctl(p.name[0] ? p.name : "tunl0", &p);
-		break;
-	case IPPROTO_GRE:
-		err = tnl_get_ioctl(p.name[0] ? p.name : "gre0", &p);
-		break;
-	case IPPROTO_IPV6:
-		err = tnl_get_ioctl(p.name[0] ? p.name : "sit0", &p);
-		break;
-	default:
-		do_tunnels_list(&p);
-		return 0;
-	}
-	if (err)
+	if (!(basedev = tnl_defname(&p)))
+		return do_tunnels_list(&p);
+
+	if (tnl_get_ioctl(p.name[0] ? p.name : basedev, &p))
 		return -1;
 
 	print_tunnel(&p);
-- 
2.1.2

[iproute PATCH 11/12] iptunnel: sanitize copying tunnel name

[Phil Sutter]

Since p->name is only IFNAMSIZ bytes, do not copy more than IFNAMSIZ - 1
bytes into it so there remains at least a single null byte in the end.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/iptunnel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 3b46a15..b377a5b 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -175,7 +175,7 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 				usage();
 			if (p->name[0])
 				duparg2("name", *argv);
-			strncpy(p->name, *argv, IFNAMSIZ);
+			strncpy(p->name, *argv, IFNAMSIZ - 1);
 			if (cmd == SIOCCHGTUNNEL && count == 0) {
 				struct ip_tunnel_parm old_p;
 				memset(&old_p, 0, sizeof(old_p));
-- 
2.1.2

[iproute PATCH 12/12] ip{,6}tunnel: put spaces around non-unary operators

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ip6tunnel.c | 16 ++++++++--------
 ip/iptunnel.c  | 40 ++++++++++++++++++++--------------------
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index d8957f0..320d253 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -110,22 +110,22 @@ static void print_tunnel(struct ip6_tnl_parm2 *p)
 		printf(" dscp inherit");
 
 	if (p->proto == IPPROTO_GRE) {
-		if ((p->i_flags&GRE_KEY) && (p->o_flags&GRE_KEY) && p->o_key == p->i_key)
+		if ((p->i_flags & GRE_KEY) && (p->o_flags & GRE_KEY) && p->o_key == p->i_key)
 			printf(" key %u", ntohl(p->i_key));
-		else if ((p->i_flags|p->o_flags)&GRE_KEY) {
-			if (p->i_flags&GRE_KEY)
+		else if ((p->i_flags | p->o_flags) & GRE_KEY) {
+			if (p->i_flags & GRE_KEY)
 				printf(" ikey %u", ntohl(p->i_key));
-			if (p->o_flags&GRE_KEY)
+			if (p->o_flags & GRE_KEY)
 				printf(" okey %u", ntohl(p->o_key));
 		}
 
-		if (p->i_flags&GRE_SEQ)
+		if (p->i_flags & GRE_SEQ)
 			printf("%s  Drop packets out of sequence.", _SL_);
-		if (p->i_flags&GRE_CSUM)
+		if (p->i_flags & GRE_CSUM)
 			printf("%s  Checksum in received packet is required.", _SL_);
-		if (p->o_flags&GRE_SEQ)
+		if (p->o_flags & GRE_SEQ)
 			printf("%s  Sequence packets on output.", _SL_);
-		if (p->o_flags&GRE_CSUM)
+		if (p->o_flags & GRE_CSUM)
 			printf("%s  Checksum output packets.", _SL_);
 	}
 }
diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index b377a5b..b9552ed 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -139,7 +139,7 @@ static int parse_args(int argc, char **argv, int cmd, struct ip_tunnel_parm *p)
 				p->iph.saddr = htonl(INADDR_ANY);
 		} else if (strcmp(*argv, "dev") == 0) {
 			NEXT_ARG();
-			strncpy(medium, *argv, IFNAMSIZ-1);
+			strncpy(medium, *argv, IFNAMSIZ - 1);
 		} else if (strcmp(*argv, "ttl") == 0 ||
 			   strcmp(*argv, "hoplimit") == 0 ||
 			   strcmp(*argv, "hlim") == 0) {
@@ -336,14 +336,14 @@ static void print_tunnel(struct ip_tunnel_parm *p)
 	if (p->iph.tos) {
 		SPRINT_BUF(b1);
 		printf(" tos");
-		if (p->iph.tos&1)
+		if (p->iph.tos & 1)
 			printf(" inherit");
-		if (p->iph.tos&~1)
-			printf("%c%s ", p->iph.tos&1 ? '/' : ' ',
-			       rtnl_dsfield_n2a(p->iph.tos&~1, b1, sizeof(b1)));
+		if (p->iph.tos & ~1)
+			printf("%c%s ", p->iph.tos & 1 ? '/' : ' ',
+			       rtnl_dsfield_n2a(p->iph.tos & ~1, b1, sizeof(b1)));
 	}
 
-	if (!(p->iph.frag_off&htons(IP_DF)))
+	if (!(p->iph.frag_off & htons(IP_DF)))
 		printf(" nopmtudisc");
 
 	if (p->iph.protocol == IPPROTO_IPV6 && !tnl_ioctl_get_6rd(p->name, &ip6rd) && ip6rd.prefixlen) {
@@ -357,22 +357,22 @@ static void print_tunnel(struct ip_tunnel_parm *p)
 		}
 	}
 
-	if ((p->i_flags&GRE_KEY) && (p->o_flags&GRE_KEY) && p->o_key == p->i_key)
+	if ((p->i_flags & GRE_KEY) && (p->o_flags & GRE_KEY) && p->o_key == p->i_key)
 		printf(" key %u", ntohl(p->i_key));
-	else if ((p->i_flags|p->o_flags)&GRE_KEY) {
-		if (p->i_flags&GRE_KEY)
+	else if ((p->i_flags | p->o_flags) & GRE_KEY) {
+		if (p->i_flags & GRE_KEY)
 			printf(" ikey %u", ntohl(p->i_key));
-		if (p->o_flags&GRE_KEY)
+		if (p->o_flags & GRE_KEY)
 			printf(" okey %u", ntohl(p->o_key));
 	}
 
-	if (p->i_flags&GRE_SEQ)
+	if (p->i_flags & GRE_SEQ)
 		printf("%s  Drop packets out of sequence.", _SL_);
-	if (p->i_flags&GRE_CSUM)
+	if (p->i_flags & GRE_CSUM)
 		printf("%s  Checksum in received packet is required.", _SL_);
-	if (p->o_flags&GRE_SEQ)
+	if (p->o_flags & GRE_SEQ)
 		printf("%s  Sequence packets on output.", _SL_);
-	if (p->o_flags&GRE_CSUM)
+	if (p->o_flags & GRE_CSUM)
 		printf("%s  Checksum output packets.", _SL_);
 }
 
@@ -592,19 +592,19 @@ int do_iptunnel(int argc, char **argv)
 
 	if (argc > 0) {
 		if (matches(*argv, "add") == 0)
-			return do_add(SIOCADDTUNNEL, argc-1, argv+1);
+			return do_add(SIOCADDTUNNEL, argc - 1, argv + 1);
 		if (matches(*argv, "change") == 0)
-			return do_add(SIOCCHGTUNNEL, argc-1, argv+1);
+			return do_add(SIOCCHGTUNNEL, argc - 1, argv + 1);
 		if (matches(*argv, "delete") == 0)
-			return do_del(argc-1, argv+1);
+			return do_del(argc - 1, argv + 1);
 		if (matches(*argv, "show") == 0 ||
 		    matches(*argv, "lst") == 0 ||
 		    matches(*argv, "list") == 0)
-			return do_show(argc-1, argv+1);
+			return do_show(argc - 1, argv + 1);
 		if (matches(*argv, "prl") == 0)
-			return do_prl(argc-1, argv+1);
+			return do_prl(argc - 1, argv + 1);
 		if (matches(*argv, "6rd") == 0)
-			return do_6rd(argc-1, argv+1);
+			return do_6rd(argc - 1, argv + 1);
 		if (matches(*argv, "help") == 0)
 			usage();
 	} else
-- 
2.1.2

RE: [iproute PATCH 05/12] ip{,6}tunnel: align do_tunnels_list() a bit

[David Laight]

From: Phil Sutter
> Sent: 13 November 2015 17:09
> In iptunnel, declare loop variables inside the loop as done in
> ip6tunnel.
...
> @@ -396,14 +396,8 @@ static void print_tunnel(struct ip_tunnel_parm *p)
> 
>  static int do_tunnels_list(struct ip_tunnel_parm *p)
>  {
> -	char name[IFNAMSIZ];
> -	unsigned long  rx_bytes, rx_packets, rx_errs, rx_drops,
> -	rx_fifo, rx_frame,
> -	tx_bytes, tx_packets, tx_errs, tx_drops,
> -	tx_fifo, tx_colls, tx_carrier, rx_multi;
> -	struct ip_tunnel_parm p1;
> -
...
>  	while (fgets(buf, sizeof(buf), fp) != NULL) {
> +		char name[IFNAMSIZ];
>  		int index, type;
> +		unsigned long rx_bytes, rx_packets, rx_errs, rx_drops,
> +			rx_fifo, rx_frame,
> +			tx_bytes, tx_packets, tx_errs, tx_drops,
> +			tx_fifo, tx_colls, tx_carrier, rx_multi;
> +		struct ip_tunnel_parm p1;
>  		char *ptr;
> +

Personally I find that just makes it harder to find where the
variables are defined.
Since the linux kernel cannot be compiled with -Wshadow declaring
variables in inner scopes can easily lead to very strange bugs.

	David

Re: [iproute PATCH 05/12] ip{,6}tunnel: align do_tunnels_list() a bit

[Phil Sutter]

On Fri, Nov 13, 2015 at 05:30:10PM +0000, David Laight wrote:
> From: Phil Sutter
> > Sent: 13 November 2015 17:09
> > In iptunnel, declare loop variables inside the loop as done in
> > ip6tunnel.
> ...
> > @@ -396,14 +396,8 @@ static void print_tunnel(struct ip_tunnel_parm *p)
> > 
> >  static int do_tunnels_list(struct ip_tunnel_parm *p)
> >  {
> > -	char name[IFNAMSIZ];
> > -	unsigned long  rx_bytes, rx_packets, rx_errs, rx_drops,
> > -	rx_fifo, rx_frame,
> > -	tx_bytes, tx_packets, tx_errs, tx_drops,
> > -	tx_fifo, tx_colls, tx_carrier, rx_multi;
> > -	struct ip_tunnel_parm p1;
> > -
> ...
> >  	while (fgets(buf, sizeof(buf), fp) != NULL) {
> > +		char name[IFNAMSIZ];
> >  		int index, type;
> > +		unsigned long rx_bytes, rx_packets, rx_errs, rx_drops,
> > +			rx_fifo, rx_frame,
> > +			tx_bytes, tx_packets, tx_errs, tx_drops,
> > +			tx_fifo, tx_colls, tx_carrier, rx_multi;
> > +		struct ip_tunnel_parm p1;
> >  		char *ptr;
> > +
> 
> Personally I find that just makes it harder to find where the
> variables are defined.

Well, the above aligns the code with ip/ip6tunnel.c in that particular
matter. I'm neither a friend of the old nor the new version, so if
everyone thinks it is better without this patch, I'm fine with changing
ip/ip6tunnel.c accordingly as well.

Looking at the code again, maybe the better option overall would be to
export the whole file reading and stats printing code into a shared
function.

> Since the linux kernel cannot be compiled with -Wshadow declaring
> variables in inner scopes can easily lead to very strange bugs.

Well, since this is not kernel code but iproute2 one, we *could* compile
it with -Wshadow.

Thanks, Phil

Re: [iproute PATCH 00/12] smaller iptunnel and ip6tunnel review

[Stephen Hemminger]

On Fri, 13 Nov 2015 18:08:53 +0100
Phil Sutter <phil@nwl.cc> wrote:

> In an effort to try and merge iptunnel and ip6tunnel support code, I found a
> few things worth changing, mainly by comparing the two files.
> 
> Please note that I did not test functionality of all supported tunnel modes,
> but since the changes are fairly small and obvious I hopefully didn't introduce
> too many bugs.
> 
> Phil Sutter (12):
>   ip{,6}tunnel: get rid of extraneous whitespace when printing
>   ip/tunnel: introduce tnl_parse_key()
>   ip{,6}tunnel: unify behaviour if physical device is not found
>   iptunnel: use ll_name_to_index() for physical interface lookup
>   ip{,6}tunnel: align do_tunnels_list() a bit
>   ip6tunnel: print local/remote addresses like iptunnel does
>   ip6tunnel: fix coding style: no newline between brace and else
>   iptunnel: share common code when setting tunnel mode
>   iptunnel: simplify parsing TTL, allow 'hlim' as identifier
>   iptunnel: share common code when determining the default interface
>     name
>   iptunnel: sanitize copying tunnel name
>   ip{,6}tunnel: put spaces around non-unary operators
> 
>  ip/ip6tunnel.c |  83 +++++++-------------
>  ip/iptunnel.c  | 239 ++++++++++++++++++++++-----------------------------------
>  ip/tunnel.c    |  15 ++++
>  ip/tunnel.h    |   1 +
>  4 files changed, 135 insertions(+), 203 deletions(-)
> 

Sure these all make sense. Applied