[iproute PATCH 0/5] warning-annoyance induced code-review

[iproute PATCH 0/5] warning-annoyance induced code-review

[Phil Sutter]

The primary goal was to get rid of the -Wunused-result warnings emitted
during compiling. While adding the necessary checks, I found a few
functions which could benefit from a bigger review (patches 1, 2 and 3).
Patch 4 then adds the remaining missing checks, and patch 5 simplifies
fgets() usage at a few spots.

Note that the last patch is my first Coccinelle-generated change, which
makes me especially proud. Many thanks again to Julia Lawall for her kind
support via IRC at this point!

Phil Sutter (5):
  lnstat: review lnstat_update()
  ss: reduce max indentation level in init_service_resolver()
  ss: review is_ephemeral()
  get rid of remaining -Wunused-result warnings
  get rid of unnecessary fgets() buffer size limitation

 misc/arpd.c        |   2 +-
 misc/ifstat.c      |   6 ++-
 misc/lnstat_util.c |  21 +++++------
 misc/nstat.c       |   6 ++-
 misc/ss.c          | 107 ++++++++++++++++++++++++++++-------------------------
 5 files changed, 74 insertions(+), 68 deletions(-)

-- 
2.5.0

[iproute PATCH 1/5] lnstat: review lnstat_update()

[Phil Sutter]

Instead of calling rewind() and fgets() before every call to
scan_lines(), move them into scan_lines() itself.

This should also fix compat mode, as before the second call to
scan_lines() the first line was skipped unconditionally.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/lnstat_util.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/misc/lnstat_util.c b/misc/lnstat_util.c
index 6dde7c4943271..433e992976eac 100644
--- a/misc/lnstat_util.c
+++ b/misc/lnstat_util.c
@@ -38,18 +38,22 @@
 /* Read (and summarize for SMP) the different stats vars. */
 static int scan_lines(struct lnstat_file *lf, int i)
 {
+	char buf[FGETS_BUF_SIZE];
 	int j, num_lines = 0;
 
 	for (j = 0; j < lf->num_fields; j++)
 		lf->fields[j].values[i] = 0;
 
-	while(!feof(lf->fp)) {
-		char buf[FGETS_BUF_SIZE];
+	rewind(lf->fp);
+	/* skip first line */
+	if (!lf->compat && !fgets(buf, sizeof(buf)-1, lf->fp))
+		return -1;
+
+	while(!feof(lf->fp) && fgets(buf, sizeof(buf)-1, lf->fp)) {
 		char *ptr = buf;
 
 		num_lines++;
 
-		fgets(buf, sizeof(buf)-1, lf->fp);
 		gettimeofday(&lf->last_read, NULL);
 
 		for (j = 0; j < lf->num_fields; j++) {
@@ -81,7 +85,6 @@ static int time_after(struct timeval *last,
 int lnstat_update(struct lnstat_file *lnstat_files)
 {
 	struct lnstat_file *lf;
-	char buf[FGETS_BUF_SIZE];
 	struct timeval tv;
 
 	gettimeofday(&tv, NULL);
@@ -91,11 +94,6 @@ int lnstat_update(struct lnstat_file *lnstat_files)
 			int i;
 			struct lnstat_field *lfi;
 
-			rewind(lf->fp);
-			if (!lf->compat) {
-				/* skip first line */
-				fgets(buf, sizeof(buf)-1, lf->fp);
-			}
 			scan_lines(lf, 1);
 
 			for (i = 0, lfi = &lf->fields[i];
@@ -107,8 +105,6 @@ int lnstat_update(struct lnstat_file *lnstat_files)
 				    			/ lf->interval.tv_sec;
 			}
 
-			rewind(lf->fp);
-			fgets(buf, sizeof(buf)-1, lf->fp);
 			scan_lines(lf, 0);
 		}
 	}
-- 
2.5.0

[iproute PATCH 2/5] ss: reduce max indentation level in init_service_resolver()

[Phil Sutter]

Exit early or continue on error instead of putting conditional into
conditional to make reading the code a bit easier.

Also, the call to memcpy() can be skipped by initialising prog with the
desired prefix.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ss.c | 53 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/misc/ss.c b/misc/ss.c
index a9ae85ec564e9..4988d34e18c78 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -870,31 +870,38 @@ static void init_service_resolver(void)
 {
 	char buf[128];
 	FILE *fp = popen("/usr/sbin/rpcinfo -p 2>/dev/null", "r");
-	if (fp) {
-		fgets(buf, sizeof(buf), fp);
-		while (fgets(buf, sizeof(buf), fp) != NULL) {
-			unsigned int progn, port;
-			char proto[128], prog[128];
-			if (sscanf(buf, "%u %*d %s %u %s", &progn, proto,
-				   &port, prog+4) == 4) {
-				struct scache *c = malloc(sizeof(*c));
-				if (c) {
-					c->port = port;
-					memcpy(prog, "rpc.", 4);
-					c->name = strdup(prog);
-					if (strcmp(proto, TCP_PROTO) == 0)
-						c->proto = TCP_PROTO;
-					else if (strcmp(proto, UDP_PROTO) == 0)
-						c->proto = UDP_PROTO;
-					else
-						c->proto = NULL;
-					c->next = rlist;
-					rlist = c;
-				}
-			}
-		}
+
+	if (!fp)
+		return;
+
+	if (!fgets(buf, sizeof(buf), fp)) {
 		pclose(fp);
+		return;
+	}
+	while (fgets(buf, sizeof(buf), fp) != NULL) {
+		unsigned int progn, port;
+		char proto[128], prog[128] = "rpc.";
+		struct scache *c;
+
+		if (sscanf(buf, "%u %*d %s %u %s",
+		           &progn, proto, &port, prog+4) != 4)
+			continue;
+
+		if (!(c = malloc(sizeof(*c))))
+			continue;
+
+		c->port = port;
+		c->name = strdup(prog);
+		if (strcmp(proto, TCP_PROTO) == 0)
+			c->proto = TCP_PROTO;
+		else if (strcmp(proto, UDP_PROTO) == 0)
+			c->proto = UDP_PROTO;
+		else
+			c->proto = NULL;
+		c->next = rlist;
+		rlist = c;
 	}
+	pclose(fp);
 }
 
 static int ip_local_port_min, ip_local_port_max;
-- 
2.5.0

[iproute PATCH 3/5] ss: review is_ephemeral()

[Phil Sutter]

No need to keep static port boundaries global, they are not used
directly. Keeping them local also allows to safely reduce their names to
the minimum. Assign hardcoded fallback values also if fscanf() fails.
Get rid of unnecessary braces around return parameter.

Instead of more or less duplicating is_ephemeral() in run_ssfilter(),
simply call the function instead.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ss.c | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/misc/ss.c b/misc/ss.c
index 4988d34e18c78..7928573285e4e 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -904,8 +904,6 @@ static void init_service_resolver(void)
 	pclose(fp);
 }
 
-static int ip_local_port_min, ip_local_port_max;
-
 /* Even do not try default linux ephemeral port ranges:
  * default /etc/services contains so much of useless crap
  * wouldbe "allocated" to this area that resolution
@@ -914,19 +912,18 @@ static int ip_local_port_min, ip_local_port_max;
  */
 static int is_ephemeral(int port)
 {
-	if (!ip_local_port_min) {
+	static int min = 0, max = 0;
+
+	if (!min) {
 		FILE *f = ephemeral_ports_open();
-		if (f) {
-			fscanf(f, "%d %d",
-			       &ip_local_port_min, &ip_local_port_max);
-			fclose(f);
-		} else {
-			ip_local_port_min = 1024;
-			ip_local_port_max = 4999;
+		if (!f || fscanf(f, "%d %d", &min, &max) < 2) {
+			min = 1024;
+			max = 4999;
 		}
+		if (f)
+			fclose(f);
 	}
-
-	return (port >= ip_local_port_min && port<= ip_local_port_max);
+	return port >= min && port <= max;
 }
 
 
@@ -1081,8 +1078,6 @@ static int run_ssfilter(struct ssfilter *f, struct sockstat *s)
 	switch (f->type) {
 		case SSF_S_AUTO:
 	{
-                static int low, high=65535;
-
 		if (s->local.family == AF_UNIX) {
 			char *p;
 			memcpy(&p, s->local.data, sizeof(p));
@@ -1094,14 +1089,7 @@ static int run_ssfilter(struct ssfilter *f, struct sockstat *s)
 		if (s->local.family == AF_NETLINK)
 			return s->lport < 0;
 
-                if (!low) {
-			FILE *fp = ephemeral_ports_open();
-			if (fp) {
-				fscanf(fp, "%d%d", &low, &high);
-				fclose(fp);
-			}
-		}
-		return s->lport >= low && s->lport <= high;
+		return is_ephemeral(s->lport);
 	}
 		case SSF_DCOND:
 	{
-- 
2.5.0

[iproute PATCH 4/5] get rid of remaining -Wunused-result warnings

[Phil Sutter]

Although not fundamentally necessary to check return codes in these
spots, preventing the warnings will put new ones into focus.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ifstat.c      |  6 ++++--
 misc/lnstat_util.c |  3 ++-
 misc/nstat.c       |  6 ++++--
 misc/ss.c          | 18 ++++++++++++++----
 4 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 20a8db45985be..ac5c29c89184a 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -819,7 +819,8 @@ int main(int argc, char *argv[])
 			}
 			if (uptime >= 0 && time(NULL) >= stb.st_mtime+uptime) {
 				fprintf(stderr, "ifstat: history is aged out, resetting\n");
-				ftruncate(fileno(hist_fp), 0);
+				if (ftruncate(fileno(hist_fp), 0))
+					perror("ifstat: ftruncate");
 			}
 		}
 
@@ -862,7 +863,8 @@ int main(int argc, char *argv[])
 	}
 
 	if (!no_update) {
-		ftruncate(fileno(hist_fp), 0);
+		if (ftruncate(fileno(hist_fp), 0))
+			perror("ifstat: ftruncate");
 		rewind(hist_fp);
 
 		json_output = 0;
diff --git a/misc/lnstat_util.c b/misc/lnstat_util.c
index 433e992976eac..70a77c5649f9b 100644
--- a/misc/lnstat_util.c
+++ b/misc/lnstat_util.c
@@ -138,7 +138,8 @@ static int lnstat_scan_fields(struct lnstat_file *lf)
 	char buf[FGETS_BUF_SIZE];
 
 	rewind(lf->fp);
-	fgets(buf, sizeof(buf)-1, lf->fp);
+	if (!fgets(buf, sizeof(buf)-1, lf->fp))
+		return -1;
 
 	return __lnstat_scan_fields(lf, buf);
 }
diff --git a/misc/nstat.c b/misc/nstat.c
index 267e515ff987c..99705286d279c 100644
--- a/misc/nstat.c
+++ b/misc/nstat.c
@@ -649,7 +649,8 @@ int main(int argc, char *argv[])
 			}
 			if (uptime >= 0 && time(NULL) >= stb.st_mtime+uptime) {
 				fprintf(stderr, "nstat: history is aged out, resetting\n");
-				ftruncate(fileno(hist_fp), 0);
+				if (ftruncate(fileno(hist_fp), 0) < 0)
+					perror("nstat: ftruncate");
 			}
 		}
 
@@ -693,7 +694,8 @@ int main(int argc, char *argv[])
 			dump_incr_db(stdout);
 	}
 	if (!no_update) {
-		ftruncate(fileno(hist_fp), 0);
+		if (ftruncate(fileno(hist_fp), 0) < 0)
+			perror("nstat: ftruncate");
 		rewind(hist_fp);
 
 		json_output = 0;
diff --git a/misc/ss.c b/misc/ss.c
index 7928573285e4e..d509009429de1 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -528,7 +528,8 @@ static void user_ent_hash_build(void)
 				snprintf(tmp, sizeof(tmp), "%s/%d/stat",
 					root, pid);
 				if ((fp = fopen(tmp, "r")) != NULL) {
-					fscanf(fp, "%*d (%[^)])", p);
+					if (fscanf(fp, "%*d (%[^)])", p) < 1)
+						; /* ignore */
 					fclose(fp);
 				}
 			}
@@ -660,7 +661,10 @@ static int get_slabstat(struct slabstat *s)
 
 	cnt = sizeof(*s)/sizeof(int);
 
-	fgets(buf, sizeof(buf), fp);
+	if (!fgets(buf, sizeof(buf), fp)) {
+		fclose(fp);
+		return -1;
+	}
 	while(fgets(buf, sizeof(buf), fp) != NULL) {
 		int i;
 		for (i=0; i<sizeof(slabstat_ids)/sizeof(slabstat_ids[0]); i++) {
@@ -2725,7 +2729,10 @@ static int unix_show(struct filter *f)
 
 	if ((fp = net_unix_open()) == NULL)
 		return -1;
-	fgets(buf, sizeof(buf)-1, fp);
+	if (!fgets(buf, sizeof(buf)-1, fp)) {
+		fclose(fp);
+		return -1;
+	}
 
 	if (memcmp(buf, "Peer", 4) == 0)
 		newformat = 1;
@@ -3210,7 +3217,10 @@ static int netlink_show(struct filter *f)
 
 	if ((fp = net_netlink_open()) == NULL)
 		return -1;
-	fgets(buf, sizeof(buf)-1, fp);
+	if (!fgets(buf, sizeof(buf)-1, fp)) {
+		fclose(fp);
+		return -1;
+	}
 
 	while (fgets(buf, sizeof(buf)-1, fp)) {
 		sscanf(buf, "%llx %d %d %x %d %d %llx %d",
-- 
2.5.0

[iproute PATCH 5/5] get rid of unnecessary fgets() buffer size limitation

[Phil Sutter]

fgets() will read at most size-1 bytes into the buffer and add a
terminating null-char at the end. Therefore it is not necessary to pass
a reduced buffer size when calling it.

This change was generated using the following semantic patch:

@@
identifier buf, fp;
@@
- fgets(buf, sizeof(buf) - 1, fp)
+ fgets(buf, sizeof(buf), fp)

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/arpd.c | 2 +-
 misc/ss.c   | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/misc/arpd.c b/misc/arpd.c
index 7919eb8b36bb7..6bb9bd16bc46c 100644
--- a/misc/arpd.c
+++ b/misc/arpd.c
@@ -703,7 +703,7 @@ int main(int argc, char **argv)
 		}
 
 		buf[sizeof(buf)-1] = 0;
-		while (fgets(buf, sizeof(buf)-1, fp)) {
+		while (fgets(buf, sizeof(buf), fp)) {
 			__u8 b1[6];
 			char ipbuf[128];
 			char macbuf[128];
diff --git a/misc/ss.c b/misc/ss.c
index d509009429de1..0dab32ceff594 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -2729,7 +2729,7 @@ static int unix_show(struct filter *f)
 
 	if ((fp = net_unix_open()) == NULL)
 		return -1;
-	if (!fgets(buf, sizeof(buf)-1, fp)) {
+	if (!fgets(buf, sizeof(buf), fp)) {
 		fclose(fp);
 		return -1;
 	}
@@ -2738,7 +2738,7 @@ static int unix_show(struct filter *f)
 		newformat = 1;
 	cnt = 0;
 
-	while (fgets(buf, sizeof(buf)-1, fp)) {
+	while (fgets(buf, sizeof(buf), fp)) {
 		struct sockstat *u, **insp;
 		int flags;
 
@@ -3217,12 +3217,12 @@ static int netlink_show(struct filter *f)
 
 	if ((fp = net_netlink_open()) == NULL)
 		return -1;
-	if (!fgets(buf, sizeof(buf)-1, fp)) {
+	if (!fgets(buf, sizeof(buf), fp)) {
 		fclose(fp);
 		return -1;
 	}
 
-	while (fgets(buf, sizeof(buf)-1, fp)) {
+	while (fgets(buf, sizeof(buf), fp)) {
 		sscanf(buf, "%llx %d %d %x %d %d %llx %d",
 		       &sk,
 		       &prot, &pid, &groups, &rq, &wq, &cb, &rc);
-- 
2.5.0

Re: [iproute PATCH 0/5] warning-annoyance induced code-review

[Stephen Hemminger]

On Sat, 28 Nov 2015 01:00:00 +0100
Phil Sutter <phil@nwl.cc> wrote:

> The primary goal was to get rid of the -Wunused-result warnings emitted
> during compiling. While adding the necessary checks, I found a few
> functions which could benefit from a bigger review (patches 1, 2 and 3).
> Patch 4 then adds the remaining missing checks, and patch 5 simplifies
> fgets() usage at a few spots.
> 
> Note that the last patch is my first Coccinelle-generated change, which
> makes me especially proud. Many thanks again to Julia Lawall for her kind
> support via IRC at this point!
> 
> Phil Sutter (5):
>   lnstat: review lnstat_update()
>   ss: reduce max indentation level in init_service_resolver()
>   ss: review is_ephemeral()
>   get rid of remaining -Wunused-result warnings
>   get rid of unnecessary fgets() buffer size limitation
> 
>  misc/arpd.c        |   2 +-
>  misc/ifstat.c      |   6 ++-
>  misc/lnstat_util.c |  21 +++++------
>  misc/nstat.c       |   6 ++-
>  misc/ss.c          | 107 ++++++++++++++++++++++++++++-------------------------
>  5 files changed, 74 insertions(+), 68 deletions(-)
> 

Applied, thanks