[PATCH iproute2-next 0/4] ip: Introduce and use helper to read /proc/net/dev

[PATCH iproute2-next 0/4] ip: Introduce and use helper to read /proc/net/dev

[Serhey Popovych]

Currently there is two places in ip(8) where /proc/net/dev is read line
by line with nearly identical steps: iptunnel.c and ip6tunnel.c

On the other hand we have iptuntap.c that uses /sys/class/net that could
be problematic in case of unshare(1)d network namespace without sysfs
being mounted.

Introduce and use do_each_proc_net_dev() helper to read data from
/proc/net/dev line by line and pass this information to implementation
specific callback function.

See individual patch description message for more details.

Series is open for reviews and comments.

Tested only by compiling and executing ip [-46] [-s] [-d] tunnel in
various combinations: no problem so far. More can be done by request.

Thanks,
Serhii

Serhey Popovych (4):
  utils: Introduce do_each_proc_net_dev() helper
  iptunnel: Use do_each_proc_net_dev()
  ip6tunnel: Use do_each_proc_net_dev()
  tuntap: Use do_each_proc_net_dev()

 include/utils.h |   10 ++++++
 ip/ip6tunnel.c  |   94 ++++++++++++++++++++------------------------------
 ip/iptunnel.c   |  102 +++++++++++++++++++++++++------------------------------
 ip/iptuntap.c   |   59 ++++++++++++++------------------
 lib/utils.c     |   51 ++++++++++++++++++++++++++++
 5 files changed, 170 insertions(+), 146 deletions(-)

-- 
1.7.10.4

[PATCH iproute2-next 1/4] utils: Introduce do_each_proc_net_dev() helper

[Serhey Popovych]

It is natural for ip(8) tool to access /proc/net/dev for various
information and there at least two places implementing same iteration
over lines in this file: iptunnel.c and ip6tunnel.c.

To unify interface and avoid code duplication introduce helper that
reads line from /proc/net/dev, passes it to given callback function
that may return following three states to control do_each_proc_net_dev()
behaviour:

  o PND_ERROR - stop reading lines and return -1
  o PND_OK    - stop reading lines and return 0
  o PND_NEXT  - continue by reading next line.

Signed-off-by: Serhey Popovych <serhe.popovych@gmail.com>
---
 include/utils.h |   10 ++++++++++
 lib/utils.c     |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/include/utils.h b/include/utils.h
index 0394268..819b0ca 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -285,6 +285,16 @@ int iplink_parse(int argc, char **argv, struct iplink_req *req,
 int do_each_netns(int (*func)(char *nsname, void *arg), void *arg,
 		bool show_label);
 
+typedef enum {
+	PND_ERROR = -1,
+	PND_OK    = 0,
+	PND_NEXT  = 1
+} pnd_result_t;
+
+typedef pnd_result_t (pnd_func_t)(char *name, char *stats, void *arg);
+
+int do_each_proc_net_dev(pnd_func_t f, void *arg);
+
 char *int_to_str(int val, char *buf);
 int get_guid(__u64 *guid, const char *arg);
 int get_real_family(int rtm_type, int rtm_family);
diff --git a/lib/utils.c b/lib/utils.c
index 8e15625..4f142d8 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1375,6 +1375,57 @@ int do_each_netns(int (*func)(char *nsname, void *arg), void *arg,
 	return netns_foreach(on_netns, &nsf);
 }
 
+int do_each_proc_net_dev(pnd_func_t f, void *arg)
+{
+	static const char pnd[] = "/proc/net/dev";
+	char buf[512];
+	int err = -1;
+	FILE *fp;
+
+	fp = fopen(pnd, "r");
+	if (!fp) {
+		perror("fopen");
+		return -1;
+	}
+
+	/* skip two lines at the begenning of the file */
+	if (!fgets(buf, sizeof(buf), fp) ||
+	    !fgets(buf, sizeof(buf), fp)) {
+		perror("fgets");
+		goto end;
+	}
+
+	while (fgets(buf, sizeof(buf), fp)) {
+		char *stats, *name = NULL;
+		pnd_result_t result;
+
+		buf[sizeof(buf) - 1] = '\0';
+		stats = strchr(buf, ':');
+		if (stats) {
+			*stats++ = '\0';
+			if (!check_ifname(buf))
+				name = buf;
+		}
+		if (!name) {
+			fprintf(stderr,
+				"Wrong format for \"%s\". Giving up.\n", pnd);
+			goto end;
+		}
+
+		result = f(name, stats, arg);
+		if (result == PND_NEXT)
+			continue;
+		if (result == PND_OK)
+			break;
+		goto end;
+	}
+	if (feof(fp))
+		err = 0;
+end:
+	fclose(fp);
+	return err;
+}
+
 char *int_to_str(int val, char *buf)
 {
 	sprintf(buf, "%d", val);
-- 
1.7.10.4

[PATCH iproute2-next 2/4] iptunnel: Use do_each_proc_net_dev()

[Serhey Popovych]

Now we have helper to iterate over entries in /proc/net/dev we can
simplify and cleanup do_tunnels_list() in ip/iptunnel.c.

While there replace printf("\n") with fputc('\n', stdout) and printf()
with fputs() where string does not contain format specifiers.

Also move tunnel parameter matching to new static helper function to
match ip6tunnel.c.

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

diff --git a/ip/iptunnel.c b/ip/iptunnel.c
index 0aa3b33..b60a7b7 100644
--- a/ip/iptunnel.c
+++ b/ip/iptunnel.c
@@ -373,66 +373,58 @@ static void print_tunnel(struct ip_tunnel_parm *p)
 		printf("%s  Checksum output packets.", _SL_);
 }
 
-static int do_tunnels_list(struct ip_tunnel_parm *p)
+/*
+ * @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)
 {
-	char buf[512];
-	int err = -1;
-	FILE *fp = fopen("/proc/net/dev", "r");
+	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));
+}
 
-	if (fp == NULL) {
-		perror("fopen");
-		return -1;
-	}
+static pnd_result_t do_tunnels_list(char *name, char *stats, void *arg)
+{
+	struct ip_tunnel_parm p1, *p = arg;
+	int index, type;
+
+	if (p->name[0] && strcmp(p->name, name))
+		return PND_NEXT;
 
-	/* skip header lines */
-	if (!fgets(buf, sizeof(buf), fp) ||
-	    !fgets(buf, sizeof(buf), fp)) {
-		fprintf(stderr, "/proc/net/dev read error\n");
-		goto end;
+	index = ll_name_to_index(name);
+	if (index <= 0)
+		return PND_NEXT;
+
+	type = ll_index_to_type(index);
+	if (type == -1) {
+		fprintf(stderr, "Failed to get type of \"%s\"\n", name);
+		return PND_NEXT;
 	}
 
-	while (fgets(buf, sizeof(buf), fp) != NULL) {
-		char name[IFNAMSIZ];
-		int index, type;
-		struct ip_tunnel_parm p1 = {};
-		char *ptr;
-
-		buf[sizeof(buf) - 1] = 0;
-		ptr = strchr(buf, ':');
-		if (ptr == NULL ||
-		    (*ptr++ = 0, sscanf(buf, "%s", name) != 1)) {
-			fprintf(stderr, "Wrong format for /proc/net/dev. Giving up.\n");
-			goto end;
-		}
-		if (p->name[0] && strcmp(p->name, name))
-			continue;
-		index = ll_name_to_index(name);
-		if (index == 0)
-			continue;
-		type = ll_index_to_type(index);
-		if (type == -1) {
-			fprintf(stderr, "Failed to get type of \"%s\"\n", name);
-			continue;
-		}
-		if (type != ARPHRD_TUNNEL && type != ARPHRD_IPGRE && type != ARPHRD_SIT)
-			continue;
-		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))
-			continue;
-		print_tunnel(&p1);
-		if (show_stats)
-			tnl_print_stats(ptr);
-		printf("\n");
+	switch (type) {
+	case ARPHRD_TUNNEL:
+	case ARPHRD_IPGRE:
+	case ARPHRD_SIT:
+		return PND_NEXT;
 	}
-	err = 0;
- end:
-	fclose(fp);
-	return err;
+
+	memset(&p1, 0, sizeof(p1));
+	if (tnl_get_ioctl(name, &p1))
+		return PND_NEXT;
+
+	if (!ip_tunnel_parm_match(p, &p1))
+		return PND_NEXT;
+
+	print_tunnel(&p1);
+	if (show_stats)
+		tnl_print_stats(stats);
+	fputc('\n', stdout);
+
+	return PND_NEXT;
 }
 
 static int do_show(int argc, char **argv)
@@ -446,7 +438,7 @@ static int do_show(int argc, char **argv)
 
 	basedev = tnl_defname(&p);
 	if (!basedev)
-		return do_tunnels_list(&p);
+		return do_each_proc_net_dev(do_tunnels_list, &p);
 
 	if (tnl_get_ioctl(p.name[0] ? p.name : basedev, &p))
 		return -1;
-- 
1.7.10.4

[PATCH iproute2-next 3/4] ip6tunnel: Use do_each_proc_net_dev()

[Serhey Popovych]

Now we have helper to iterate over entries in /proc/net/dev we can
simplify and cleanup do_tunnels_list() in ip/iptunnel.c.

While there replace printf("\n") with fputc('\n', stdout) and printf()
with fputs() where string does not contain format specifiers.

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

diff --git a/ip/ip6tunnel.c b/ip/ip6tunnel.c
index 783e28a..ccd5603 100644
--- a/ip/ip6tunnel.c
+++ b/ip/ip6tunnel.c
@@ -336,68 +336,48 @@ static int ip6_tnl_parm_match(const struct ip6_tnl_parm2 *p1,
 		(!p1->flags || (p1->flags & p2->flags)));
 }
 
-static int do_tunnels_list(struct ip6_tnl_parm2 *p)
+static pnd_result_t do_tunnels_list(char *name, char *stats, void *arg)
 {
-	char buf[512];
-	int err = -1;
-	FILE *fp = fopen("/proc/net/dev", "r");
+	struct ip6_tnl_parm2 p1, *p = arg;
+	int index, type;
 
-	if (fp == NULL) {
-		perror("fopen");
-		return -1;
-	}
+	if (p->name[0] && strcmp(p->name, name))
+		return PND_NEXT;
+
+	index = ll_name_to_index(name);
+	if (index <= 0)
+		return PND_NEXT;
 
-	/* 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");
-		goto end;
+	type = ll_index_to_type(index);
+	if (type == -1) {
+		fprintf(stderr, "Failed to get type of \"%s\"\n", name);
+		return PND_NEXT;
 	}
 
-	while (fgets(buf, sizeof(buf), fp) != NULL) {
-		char name[IFNAMSIZ];
-		int index, type;
-		struct ip6_tnl_parm2 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");
-			goto end;
-		}
-		if (p->name[0] && strcmp(p->name, name))
-			continue;
-		index = ll_name_to_index(name);
-		if (index == 0)
-			continue;
-		type = ll_index_to_type(index);
-		if (type == -1) {
-			fprintf(stderr, "Failed to get type of \"%s\"\n", name);
-			continue;
-		}
-		if (type != ARPHRD_TUNNEL6 && type != ARPHRD_IP6GRE)
-			continue;
-		ip6_tnl_parm_init(&p1, 0);
-		if (type == ARPHRD_IP6GRE)
-			p1.proto = IPPROTO_GRE;
-		strcpy(p1.name, name);
-		p1.link = ll_name_to_index(p1.name);
-		if (p1.link == 0)
-			continue;
-		if (tnl_get_ioctl(p1.name, &p1))
-			continue;
-		if (!ip6_tnl_parm_match(p, &p1))
-			continue;
-		print_tunnel(&p1);
-		if (show_stats)
-			tnl_print_stats(ptr);
-		printf("\n");
+	switch (type) {
+	case ARPHRD_TUNNEL6:
+	case ARPHRD_IP6GRE:
+		return PND_NEXT;
 	}
-	err = 0;
- end:
-	fclose(fp);
-	return err;
+
+	ip6_tnl_parm_init(&p1, 0);
+	if (type == ARPHRD_IP6GRE)
+		p1.proto = IPPROTO_GRE;
+	p1.link = index;
+	name = strcpy(p1.name, name);
+
+	if (tnl_get_ioctl(name, &p1))
+		return PND_NEXT;
+
+	if (!ip6_tnl_parm_match(p, &p1))
+		return PND_NEXT;
+
+	print_tunnel(&p1);
+	if (show_stats)
+		tnl_print_stats(stats);
+	fputc('\n', stdout);
+
+	return PND_NEXT;
 }
 
 static int do_show(int argc, char **argv)
@@ -412,7 +392,7 @@ static int do_show(int argc, char **argv)
 		return -1;
 
 	if (!p.name[0] || show_stats)
-		do_tunnels_list(&p);
+		do_each_proc_net_dev(do_tunnels_list, &p);
 	else {
 		if (tnl_get_ioctl(p.name, &p))
 			return -1;
-- 
1.7.10.4

[PATCH iproute2-next 4/4] tuntap: Use do_each_proc_net_dev()

[Serhey Popovych]

Now we have helper to iterate over entries in /proc/net/dev we can
simplify and cleanup do_tunnels_list() in ip/iptuntap.c.

While there replace printf("\n") with fputc('\n', stdout) and
printf() with fputs() where string does not contain format specifiers.

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

diff --git a/ip/iptuntap.c b/ip/iptuntap.c
index 09f2be2..01b68ad 100644
--- a/ip/iptuntap.c
+++ b/ip/iptuntap.c
@@ -348,44 +348,35 @@ next:
 	globfree(&globbuf);
 }
 
-
-static int do_show(int argc, char **argv)
+static pnd_result_t do_tuntap_list(char *name, char *stats, void *arg)
 {
-	DIR *dir;
-	struct dirent *d;
 	long flags, owner = -1, group = -1;
 
-	dir = opendir("/sys/class/net");
-	if (!dir) {
-		perror("opendir");
-		return -1;
+	if (read_prop(name, "tun_flags", &flags))
+		return PND_NEXT;
+
+	read_prop(name, "owner", &owner);
+	read_prop(name, "group", &group);
+
+	printf("%s:", name);
+	print_flags(flags);
+	if (owner != -1)
+		printf(" user %ld", owner);
+	if (group != -1)
+		printf(" group %ld", group);
+	fputc('\n', stdout);
+	if (show_details) {
+		fputs("\tAttached to processes:", stdout);
+		show_processes(name);
+		fputc('\n', stdout);
 	}
-	while ((d = readdir(dir))) {
-		if (d->d_name[0] == '.' &&
-		    (d->d_name[1] == 0 || d->d_name[1] == '.'))
-			continue;
-
-		if (read_prop(d->d_name, "tun_flags", &flags))
-			continue;
-
-		read_prop(d->d_name, "owner", &owner);
-		read_prop(d->d_name, "group", &group);
-
-		printf("%s:", d->d_name);
-		print_flags(flags);
-		if (owner != -1)
-			printf(" user %ld", owner);
-		if (group != -1)
-			printf(" group %ld", group);
-		printf("\n");
-		if (show_details) {
-			printf("\tAttached to processes:");
-			show_processes(d->d_name);
-			printf("\n");
-		}
-	}
-	closedir(dir);
-	return 0;
+
+	return PND_NEXT;
+}
+
+static int do_show(int argc, char **argv)
+{
+	return do_each_proc_net_dev(do_tuntap_list, NULL);
 }
 
 int do_iptuntap(int argc, char **argv)
-- 
1.7.10.4

Re: [PATCH iproute2-next 0/4] ip: Introduce and use helper to read /proc/net/dev

[Stephen Hemminger]

On Wed, 31 Jan 2018 21:49:45 +0200
Serhey Popovych <serhe.popovych@gmail.com> wrote:

> Currently there is two places in ip(8) where /proc/net/dev is read line
> by line with nearly identical steps: iptunnel.c and ip6tunnel.c
> 
> On the other hand we have iptuntap.c that uses /sys/class/net that could
> be problematic in case of unshare(1)d network namespace without sysfs
> being mounted.
> 
> Introduce and use do_each_proc_net_dev() helper to read data from
> /proc/net/dev line by line and pass this information to implementation
> specific callback function.
> 
> See individual patch description message for more details.
> 
> Series is open for reviews and comments.
> 
> Tested only by compiling and executing ip [-46] [-s] [-d] tunnel in
> various combinations: no problem so far. More can be done by request.
> 
> Thanks,
> Serhii
> 
> Serhey Popovych (4):
>   utils: Introduce do_each_proc_net_dev() helper
>   iptunnel: Use do_each_proc_net_dev()
>   ip6tunnel: Use do_each_proc_net_dev()
>   tuntap: Use do_each_proc_net_dev()
> 
>  include/utils.h |   10 ++++++
>  ip/ip6tunnel.c  |   94 ++++++++++++++++++++------------------------------
>  ip/iptunnel.c   |  102 +++++++++++++++++++++++++------------------------------
>  ip/iptuntap.c   |   59 ++++++++++++++------------------
>  lib/utils.c     |   51 ++++++++++++++++++++++++++++
>  5 files changed, 170 insertions(+), 146 deletions(-)
> 

/proc/net/dev is legacy and unextensible.

I would rather see netlink used everywhere and not /proc/net/dev or sysfs!

Re: [PATCH iproute2-next 0/4] ip: Introduce and use helper to read /proc/net/dev

[David Ahern]

On 1/31/18 4:04 PM, Stephen Hemminger wrote:
> 
> /proc/net/dev is legacy and unextensible.
> 
> I would rather see netlink used everywhere and not /proc/net/dev or sysfs!
> 

agreed. ll_init_map is already called in some places so a device cache
already exists.

Re: [PATCH iproute2-next 0/4] ip: Introduce and use helper to read /proc/net/dev

[Serhey Popovych]

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

Stephen Hemminger wrote:
> On Wed, 31 Jan 2018 21:49:45 +0200
> Serhey Popovych <serhe.popovych@gmail.com> wrote:
> 
>> Currently there is two places in ip(8) where /proc/net/dev is read line
>> by line with nearly identical steps: iptunnel.c and ip6tunnel.c
>>
>> On the other hand we have iptuntap.c that uses /sys/class/net that could
>> be problematic in case of unshare(1)d network namespace without sysfs
>> being mounted.
>>
>> Introduce and use do_each_proc_net_dev() helper to read data from
>> /proc/net/dev line by line and pass this information to implementation
>> specific callback function.
>>
>> See individual patch description message for more details.
>>
>> Series is open for reviews and comments.
>>
>> Tested only by compiling and executing ip [-46] [-s] [-d] tunnel in
>> various combinations: no problem so far. More can be done by request.
>>
>> Thanks,
>> Serhii
>>
>> Serhey Popovych (4):
>>   utils: Introduce do_each_proc_net_dev() helper
>>   iptunnel: Use do_each_proc_net_dev()
>>   ip6tunnel: Use do_each_proc_net_dev()
>>   tuntap: Use do_each_proc_net_dev()
>>
>>  include/utils.h |   10 ++++++
>>  ip/ip6tunnel.c  |   94 ++++++++++++++++++++------------------------------
>>  ip/iptunnel.c   |  102 +++++++++++++++++++++++++------------------------------
>>  ip/iptuntap.c   |   59 ++++++++++++++------------------
>>  lib/utils.c     |   51 ++++++++++++++++++++++++++++
>>  5 files changed, 170 insertions(+), 146 deletions(-)
>>
> 
> /proc/net/dev is legacy and unextensible.
> 
> I would rather see netlink used everywhere and not /proc/net/dev or sysfs!
> 

Look at my RFC series against iproute2-next with cover letter subject:
"ip: Use netlink to walk through network device list".

Thanks,
Serhii





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