diff mbox series

[iproute2-next,4/6] iptunnel/ip6tunnel: Code cleanups

Message ID 1517577055-23788-5-git-send-email-serhe.popovych@gmail.com
State Changes Requested, archived
Delegated to: David Ahern
Headers show
Series ip: Use netlink to walk through network device list | expand

Commit Message

Serhey Popovych Feb. 2, 2018, 1:10 p.m. UTC 
Use switch () instead of if () to compare tunnel type to fit into 80
columns and make code more readable. Print "\n" using fputc().

In iptunnel.c abstract tunnel parameters matching code in iptunnel.c
into ip_tunnel_parm_match() helper to conform with ip6tunnel.c.

In ip6tunnel.c no need to call ll_name_to_index() with name twice: just
use found previously index. Do not initialize @p1.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 ip/ip6tunnel.c |   30 ++++++++++++++++--------------
 ip/iptunnel.c  |   35 ++++++++++++++++++++++++++---------
 2 files changed, 42 insertions(+), 23 deletions(-)

Comments

David Ahern Feb. 7, 2018, 2:16 a.m. UTC | #1 
On 2/2/18 6:10 AM, Serhey Popovych wrote:
> @@ -414,15 +428,18 @@ static int do_tunnels_list(struct ip_tunnel_parm *p)
>  			fprintf(stderr, "Failed to get type of \"%s\"\n", name);
>  			continue;
>  		}
> -		if (type != ARPHRD_TUNNEL && type != ARPHRD_IPGRE && type != ARPHRD_SIT)
> +		switch (type) {
> +		case ARPHRD_TUNNEL:
> +		case ARPHRD_IPGRE:
> +		case ARPHRD_SIT:
> +			break;
> +		default:
>  			continue;
> +		}
> +		memset(p1, 0, sizeof(p1));

Shouldn't that be &p1 for the first arg? I get a compile failure:

ip
    CC       iptunnel.o
    CC       ip6tunnel.o
iptunnel.c: In function ‘do_tunnels_list’:
iptunnel.c:439:10: error: incompatible type for argument 1 of ‘memset’
   memset(p1, 0, sizeof(p1));
          ^~
In file included from iptunnel.c:15:0:
/usr/include/string.h:62:14: note: expected ‘void *’ but argument is of
type ‘struct ip_tunnel_parm’
 extern void *memset (void *__s, int __c, size_t __n) __THROW __nonnull
((1));
              ^~~~~~
../config.mk:48: recipe for target 'iptunnel.o' failed

>  		if (tnl_get_ioctl(name, &p1))
>  			continue;
> -		if ((p->link && p1.link != p->link) ||
> -		    (p->name[0] && strcmp(p1.name, p->name)) ||
> -		    (p->iph.daddr && p1.iph.daddr != p->iph.daddr) ||
> -		    (p->iph.saddr && p1.iph.saddr != p->iph.saddr) ||
> -		    (p->i_key && p1.i_key != p->i_key))
> +		if (!ip_tunnel_parm_match(p, &p1))
>  			continue;
>  		print_tunnel(&p1);
>  		if (show_stats) {
David Ahern Feb. 7, 2018, 2:19 a.m. UTC | #2 
On 2/6/18 7:16 PM, David Ahern wrote:
> On 2/2/18 6:10 AM, Serhey Popovych wrote:
>> @@ -414,15 +428,18 @@ static int do_tunnels_list(struct ip_tunnel_parm *p)
>>  			fprintf(stderr, "Failed to get type of \"%s\"\n", name);
>>  			continue;
>>  		}
>> -		if (type != ARPHRD_TUNNEL && type != ARPHRD_IPGRE && type != ARPHRD_SIT)
>> +		switch (type) {
>> +		case ARPHRD_TUNNEL:
>> +		case ARPHRD_IPGRE:
>> +		case ARPHRD_SIT:
>> +			break;
>> +		default:
>>  			continue;
>> +		}
>> +		memset(p1, 0, sizeof(p1));
> 
> Shouldn't that be &p1 for the first arg? I get a compile failure:
> 
> ip
>     CC       iptunnel.o
>     CC       ip6tunnel.o
> iptunnel.c: In function ‘do_tunnels_list’:
> iptunnel.c:439:10: error: incompatible type for argument 1 of ‘memset’
>    memset(p1, 0, sizeof(p1));
>           ^~
> In file included from iptunnel.c:15:0:
> /usr/include/string.h:62:14: note: expected ‘void *’ but argument is of
> type ‘struct ip_tunnel_parm’
>  extern void *memset (void *__s, int __c, size_t __n) __THROW __nonnull
> ((1));
>               ^~~~~~
> ../config.mk:48: recipe for target 'iptunnel.o' failed
> 

Fixed by patch 5 which deletes do_tunnels_list. So why have a cleanup
patch that changes code you then delete?
Serhey Popovych Feb. 7, 2018, 6:15 a.m. UTC | #3 
David Ahern wrote:
> On 2/2/18 6:10 AM, Serhey Popovych wrote:
>> @@ -414,15 +428,18 @@ static int do_tunnels_list(struct ip_tunnel_parm *p)
>>  			fprintf(stderr, "Failed to get type of \"%s\"\n", name);
>>  			continue;
>>  		}
>> -		if (type != ARPHRD_TUNNEL && type != ARPHRD_IPGRE && type != ARPHRD_SIT)
>> +		switch (type) {
>> +		case ARPHRD_TUNNEL:
>> +		case ARPHRD_IPGRE:
>> +		case ARPHRD_SIT:
>> +			break;
>> +		default:
>>  			continue;
>> +		}
>> +		memset(p1, 0, sizeof(p1));
> 
> Shouldn't that be &p1 for the first arg? I get a compile failure:

Yes, definitely, sorry for that. Will fix in v2.

> 
> ip
>     CC       iptunnel.o
>     CC       ip6tunnel.o
> iptunnel.c: In function ‘do_tunnels_list’:
> iptunnel.c:439:10: error: incompatible type for argument 1 of ‘memset’
>    memset(p1, 0, sizeof(p1));
>           ^~
> In file included from iptunnel.c:15:0:
> /usr/include/string.h:62:14: note: expected ‘void *’ but argument is of
> type ‘struct ip_tunnel_parm’
>  extern void *memset (void *__s, int __c, size_t __n) __THROW __nonnull
> ((1));
>               ^~~~~~
> ../config.mk:48: recipe for target 'iptunnel.o' failed
> 
>>  		if (tnl_get_ioctl(name, &p1))
>>  			continue;
>> -		if ((p->link && p1.link != p->link) ||
>> -		    (p->name[0] && strcmp(p1.name, p->name)) ||
>> -		    (p->iph.daddr && p1.iph.daddr != p->iph.daddr) ||
>> -		    (p->iph.saddr && p1.iph.saddr != p->iph.saddr) ||
>> -		    (p->i_key && p1.i_key != p->i_key))
>> +		if (!ip_tunnel_parm_match(p, &p1))
>>  			continue;
>>  		print_tunnel(&p1);
>>  		if (show_stats) {
Serhey Popovych Feb. 7, 2018, 6:20 a.m. UTC | #4 
David Ahern wrote:
> On 2/6/18 7:16 PM, David Ahern wrote:
>> On 2/2/18 6:10 AM, Serhey Popovych wrote:
>>> @@ -414,15 +428,18 @@ static int do_tunnels_list(struct ip_tunnel_parm *p)
>>>  			fprintf(stderr, "Failed to get type of \"%s\"\n", name);
>>>  			continue;
>>>  		}
>>> -		if (type != ARPHRD_TUNNEL && type != ARPHRD_IPGRE && type != ARPHRD_SIT)
>>> +		switch (type) {
>>> +		case ARPHRD_TUNNEL:
>>> +		case ARPHRD_IPGRE:
>>> +		case ARPHRD_SIT:
>>> +			break;
>>> +		default:
>>>  			continue;
>>> +		}
>>> +		memset(p1, 0, sizeof(p1));
>>
>> Shouldn't that be &p1 for the first arg? I get a compile failure:
>>
>> ip
>>     CC       iptunnel.o
>>     CC       ip6tunnel.o
>> iptunnel.c: In function ‘do_tunnels_list’:
>> iptunnel.c:439:10: error: incompatible type for argument 1 of ‘memset’
>>    memset(p1, 0, sizeof(p1));
>>           ^~
>> In file included from iptunnel.c:15:0:
>> /usr/include/string.h:62:14: note: expected ‘void *’ but argument is of
>> type ‘struct ip_tunnel_parm’
>>  extern void *memset (void *__s, int __c, size_t __n) __THROW __nonnull
>> ((1));
>>               ^~~~~~
>> ../config.mk:48: recipe for target 'iptunnel.o' failed
>>
> 
> Fixed by patch 5 which deletes do_tunnels_list. So why have a cleanup
> patch that changes code you then delete?
> 

There at least two reasons:

  1) Abstract tunnel matching code into a function that will be used as
     callback ->match() in upcoming change.

  2) Make do_tunnels_list() ip and ipv6 variants diff contain only
     real differences to show upcoming change where common
     do_tunnels_list() introduced is correct.

Will update comment for this patch in v2.
diff mbox series

Patch

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index 3e99559..0e53c23 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -357,7 +357,7 @@  static int do_tunnels_list(struct ip6_tnl_parm2 *p)
 	while (fgets(buf, sizeof(buf), fp) != NULL) {
 		char name[IFNAMSIZ];
 		int index, type;
-		struct ip6_tnl_parm2 p1 = {};
+		struct ip6_tnl_parm2 p1;
 		char *ptr;
 
 		buf[sizeof(buf) - 1] = '\0';
@@ -376,16 +376,19 @@  static int do_tunnels_list(struct ip6_tnl_parm2 *p)
 			fprintf(stderr, "Failed to get type of \"%s\"\n", name);
 			continue;
 		}
-		if (type != ARPHRD_TUNNEL6 && type != ARPHRD_IP6GRE)
+		switch (type) {
+		case ARPHRD_TUNNEL6:
+		case ARPHRD_IP6GRE:
+			break;
+		default:
 			continue;
+		}
 		ip6_tnl_parm_init(&p1, 0);
 		if (type == ARPHRD_IP6GRE)
 			p1.proto = IPPROTO_GRE;
+		p1.link = index;
 		strcpy(p1.name, name);
-		p1.link = ll_name_to_index(p1.name);
-		if (p1.link == 0)
-			continue;
-		if (tnl_get_ioctl(p1.name, &p1))
+		if (tnl_get_ioctl(name, &p1))
 			continue;
 		if (!ip6_tnl_parm_match(p, &p1))
 			continue;
@@ -396,7 +399,7 @@  static int do_tunnels_list(struct ip6_tnl_parm2 *p)
 			if (!tnl_get_stats(ptr, &s))
 				tnl_print_stats(&s);
 		}
-		printf("\n");
+		fputc('\n', stdout);
 	}
 	err = 0;
  end:
@@ -416,14 +419,13 @@  static int do_show(int argc, char **argv)
 		return -1;
 
 	if (!p.name[0] || show_stats)
-		do_tunnels_list(&p);
-	else {
-		if (tnl_get_ioctl(p.name, &p))
-			return -1;
-		print_tunnel(&p);
-		printf("\n");
-	}
+		return do_tunnels_list(&p);
+
+	if (tnl_get_ioctl(p.name, &p))
+		return -1;
 
+	print_tunnel(&p);
+	fputc('\n', stdout);
 	return 0;
 }
 
diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 6639055..dba5942 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -373,6 +373,20 @@  static void print_tunnel(struct ip_tunnel_parm *p)
 		printf("%s  Checksum output packets.", _SL_);
 }
 
+/*
+ * @p1: user specified parameter
+ * @p2: database entry
+ */
+static int ip_tunnel_parm_match(const struct ip_tunnel_parm *p1,
+				const struct ip_tunnel_parm *p2)
+{
+	return ((!p1->link || p1->link == p2->link) &&
+		(!p1->name[0] || strcmp(p1->name, p2->name) == 0) &&
+		(!p1->iph.daddr || p1->iph.daddr == p2->iph.daddr) &&
+		(!p1->iph.saddr || p1->iph.saddr == p2->iph.saddr) &&
+		(!p1->i_key || p1->i_key == p2->i_key));
+}
+
 static int do_tunnels_list(struct ip_tunnel_parm *p)
 {
 	char buf[512];
@@ -394,7 +408,7 @@  static int do_tunnels_list(struct ip_tunnel_parm *p)
 	while (fgets(buf, sizeof(buf), fp) != NULL) {
 		char name[IFNAMSIZ];
 		int index, type;
-		struct ip_tunnel_parm p1 = {};
+		struct ip_tunnel_parm p1;
 		char *ptr;
 
 		buf[sizeof(buf) - 1] = 0;
@@ -414,15 +428,18 @@  static int do_tunnels_list(struct ip_tunnel_parm *p)
 			fprintf(stderr, "Failed to get type of \"%s\"\n", name);
 			continue;
 		}
-		if (type != ARPHRD_TUNNEL && type != ARPHRD_IPGRE && type != ARPHRD_SIT)
+		switch (type) {
+		case ARPHRD_TUNNEL:
+		case ARPHRD_IPGRE:
+		case ARPHRD_SIT:
+			break;
+		default:
 			continue;
+		}
+		memset(p1, 0, sizeof(p1));
 		if (tnl_get_ioctl(name, &p1))
 			continue;
-		if ((p->link && p1.link != p->link) ||
-		    (p->name[0] && strcmp(p1.name, p->name)) ||
-		    (p->iph.daddr && p1.iph.daddr != p->iph.daddr) ||
-		    (p->iph.saddr && p1.iph.saddr != p->iph.saddr) ||
-		    (p->i_key && p1.i_key != p->i_key))
+		if (!ip_tunnel_parm_match(p, &p1))
 			continue;
 		print_tunnel(&p1);
 		if (show_stats) {
@@ -431,7 +448,7 @@  static int do_tunnels_list(struct ip_tunnel_parm *p)
 			if (!tnl_get_stats(ptr, &s))
 				tnl_print_stats(&s);
 		}
-		printf("\n");
+		fputc('\n', stdout);
 	}
 	err = 0;
  end:
@@ -456,7 +473,7 @@  static int do_show(int argc, char **argv)
 		return -1;
 
 	print_tunnel(&p);
-	printf("\n");
+	fputc('\n', stdout);
 	return 0;
 }