Re: [PATCH 26/28] Avoid gcc 5.4 warning -Wunused-result

Yishai Hadas <yishaih@xxxxxxxxxxxxxxxxxx> · Sun, 9 Oct 2016 16:31:17 +0300

On 9/6/2016 12:08 AM, Jason Gunthorpe wrote:
It used to be you could suppress this with (void), however the gcc
developers have decided to get rid of that.

So, look closely at each occurrence and decide what to do:
- *pingpong: Join the error handling with the if statement directly
   above
- niegh: read on a timer_fd should never fail, so just use assert.
   The assert is compiled out for Release builds so this is no-change
- acm: Failure of ucma_set_server_port is detected by a 0 return
   so check fscanf and return appropriately. This is no change since
   fscanf failure was assumed to have left server_port as 0 (though
   I doubt the standard supports that usage)
- rsocket: This looks super sketchy. At least lets make the intent clear
   with a read_all/write_all wrapper that calls assert. Most likely
   this code is wrong..
   Mangle the code with failable_fscanf to make it clear, but as with
   acm, I don't think the standard supports this usage.

Signed-off-by: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx>


This patch breaks rc/uc/ud/srq pingpong in a basic run, please see below.

Can you please prepare some fix for that as it was already merged ?

---
 libibverbs/examples/rc_pingpong.c  | 15 +++++-----
 libibverbs/examples/srq_pingpong.c | 13 +++++++--
 libibverbs/examples/uc_pingpong.c  | 15 +++++-----
 libibverbs/examples/ud_pingpong.c  | 17 +++++-------
 libibverbs/src/neigh.c             |  7 +++--
 librdmacm/src/acm.c                |  3 +-
 librdmacm/src/rsocket.c            | 56 +++++++++++++++++++++++++++-----------
 7 files changed, 78 insertions(+), 48 deletions(-)

diff --git a/libibverbs/examples/rc_pingpong.c b/libibverbs/examples/rc_pingpong.c
index 1fad16a0be7c..7bcc413a0f1d 100644
--- a/libibverbs/examples/rc_pingpong.c
+++ b/libibverbs/examples/rc_pingpong.c
@@ -208,14 +208,13 @@ static struct pingpong_dest *pp_client_exch_dest(const char *servername, int por
 		goto out;
 	}

-	if (read(sockfd, msg, sizeof msg) != sizeof msg) {
-		perror("client read");
-		fprintf(stderr, "Couldn't read remote address\n");
+	if (read(sockfd, msg, sizeof msg) != sizeof msg ||
+	    write(sockfd, "done", sizeof "done") != sizeof "done") {
+		perror("client read/write");
+		fprintf(stderr, "Couldn't read/write remote address\n");
 		goto out;
 	}

-	write(sockfd, "done", sizeof "done");
-
 	rem_dest = malloc(sizeof *rem_dest);
 	if (!rem_dest)
 		goto out;
@@ -316,14 +315,14 @@ static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx,
 	gid_to_wire_gid(&my_dest->gid, gid);
 	sprintf(msg, "%04x:%06x:%06x:%s", my_dest->lid, my_dest->qpn,
 							my_dest->psn, gid);
-	if (write(connfd, msg, sizeof msg) != sizeof msg) {
-		fprintf(stderr, "Couldn't send local address\n");
+	if (write(connfd, msg, sizeof msg) != sizeof msg ||
+	    read(connfd, msg, sizeof msg) != sizeof msg) {
+		fprintf(stderr, "Couldn't send/recv local address\n");

At that step the server expects to read a "done" response from its 
client, checking whether the read was done for sizeof msg which is much 
larger will fail.

Same issue appears below in several places in srq/ud/uc pingpong.

 		free(rem_dest);
 		rem_dest = NULL;
 		goto out;
 	}

-	read(connfd, msg, sizeof msg);

 out:
 	close(connfd);
diff --git a/libibverbs/examples/srq_pingpong.c b/libibverbs/examples/srq_pingpong.c
index 929b736545c7..e6492dc553fd 100644
--- a/libibverbs/examples/srq_pingpong.c
+++ b/libibverbs/examples/srq_pingpong.c
@@ -222,8 +222,10 @@ static struct pingpong_dest *pp_client_exch_dest(const char *servername, int por
 		wire_gid_to_gid(gid, &rem_dest[i].gid);
 	}

-	write(sockfd, "done", sizeof "done");
-
+	if (write(sockfd, "done", sizeof "done") != sizeof "done") {
+		perror("client write");
+		goto out;
+	}
 out:
 	close(sockfd);
 	return rem_dest;
@@ -333,7 +335,12 @@ static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx,
 		}
 	}

-	read(connfd, msg, sizeof msg);
+	if (read(connfd, msg, sizeof msg) != sizeof msg) {
+		perror("client write");
+		free(rem_dest);
+		rem_dest = NULL;
+		goto out;
+	}

 out:
 	close(connfd);
diff --git a/libibverbs/examples/uc_pingpong.c b/libibverbs/examples/uc_pingpong.c
index 3802e3821773..d132de98694a 100644
--- a/libibverbs/examples/uc_pingpong.c
+++ b/libibverbs/examples/uc_pingpong.c
@@ -176,13 +176,13 @@ static struct pingpong_dest *pp_client_exch_dest(const char *servername, int por
 		goto out;
 	}

-	if (read(sockfd, msg, sizeof msg) != sizeof msg) {
-		perror("client read");
-		fprintf(stderr, "Couldn't read remote address\n");
+	if (read(sockfd, msg, sizeof msg) != sizeof msg ||
+	    write(sockfd, "done", sizeof "done") != sizeof "done") {
+		perror("client read/write");
+		fprintf(stderr, "Couldn't read/write remote address\n");
 		goto out;
 	}

-	write(sockfd, "done", sizeof "done");

 	rem_dest = malloc(sizeof *rem_dest);
 	if (!rem_dest)
@@ -284,15 +284,14 @@ static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx,
 	gid_to_wire_gid(&my_dest->gid, gid);
 	sprintf(msg, "%04x:%06x:%06x:%s", my_dest->lid, my_dest->qpn,
 							my_dest->psn, gid);
-	if (write(connfd, msg, sizeof msg) != sizeof msg) {
-		fprintf(stderr, "Couldn't send local address\n");
+	if (write(connfd, msg, sizeof msg) != sizeof msg ||
+	    read(connfd, msg, sizeof msg) != sizeof msg) {
+		fprintf(stderr, "Couldn't send/recv local address\n");
 		free(rem_dest);
 		rem_dest = NULL;
 		goto out;
 	}

-	read(connfd, msg, sizeof msg);
-
 out:
 	close(connfd);
 	return rem_dest;
diff --git a/libibverbs/examples/ud_pingpong.c b/libibverbs/examples/ud_pingpong.c
index fa99b9e51dfb..67da4bd90f32 100644
--- a/libibverbs/examples/ud_pingpong.c
+++ b/libibverbs/examples/ud_pingpong.c
@@ -176,14 +176,13 @@ static struct pingpong_dest *pp_client_exch_dest(const char *servername, int por
 		goto out;
 	}

-	if (read(sockfd, msg, sizeof msg) != sizeof msg) {
-		perror("client read");
-		fprintf(stderr, "Couldn't read remote address\n");
+	if (read(sockfd, msg, sizeof msg) != sizeof msg ||
+	    write(sockfd, "done", sizeof "done") != sizeof "done") {
+		perror("client read/write");
+		fprintf(stderr, "Couldn't read/write remote address\n");
 		goto out;
 	}

-	write(sockfd, "done", sizeof "done");
-
 	rem_dest = malloc(sizeof *rem_dest);
 	if (!rem_dest)
 		goto out;
@@ -282,15 +281,13 @@ static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx,
 	gid_to_wire_gid(&my_dest->gid, gid);
 	sprintf(msg, "%04x:%06x:%06x:%s", my_dest->lid, my_dest->qpn,
 							my_dest->psn, gid);
-	if (write(connfd, msg, sizeof msg) != sizeof msg) {
-		fprintf(stderr, "Couldn't send local address\n");
+	if (write(connfd, msg, sizeof msg) != sizeof msg ||
+	    read(connfd, msg, sizeof msg) != sizeof msg) {
+		fprintf(stderr, "Couldn't send/recv local address\n");
 		free(rem_dest);
 		rem_dest = NULL;
 		goto out;
 	}
-
-	read(connfd, msg, sizeof msg);
-
 out:
 	close(connfd);
 	return rem_dest;
diff --git a/libibverbs/src/neigh.c b/libibverbs/src/neigh.c
index 6b6e58cd52f8..5acfcf06fcde 100644
--- a/libibverbs/src/neigh.c
+++ b/libibverbs/src/neigh.c
@@ -19,6 +19,7 @@
 #include <unistd.h>
 #include <ifaddrs.h>
 #include <netdb.h>
+#include <assert.h>
 #ifndef _LINUX_IF_H
 #include <net/if.h>
 #else
@@ -372,9 +373,11 @@ static struct nl_addr *process_get_neigh_mac(

 			if (FD_ISSET(timer_fd, &fdset)) {
 				uint64_t read_val;
+				ssize_t rc;

-				(void)read(timer_fd, &read_val,
-					   sizeof(read_val));
+				rc =
+				    read(timer_fd, &read_val, sizeof(read_val));
+				assert(rc == sizeof(read_val));
 				if (++retries >=  NUM_OF_TRIES) {
 					if (!errno)
 						errno = EDESTADDRREQ;
diff --git a/librdmacm/src/acm.c b/librdmacm/src/acm.c
index f0da01e6d286..c097bb923b55 100644
--- a/librdmacm/src/acm.c
+++ b/librdmacm/src/acm.c
@@ -121,7 +121,8 @@ static int ucma_set_server_port(void)
 	FILE *f;

 	if ((f = fopen("/var/run/ibacm.port", "r" STREAM_CLOEXEC))) {
-		fscanf(f, "%" SCNu16, &server_port);
+		if (fscanf(f, "%" SCNu16, &server_port) != 1)
+			server_port = 0;
 		fclose(f);
 	}
 	return server_port;
diff --git a/librdmacm/src/rsocket.c b/librdmacm/src/rsocket.c
index 818505fbe02e..5645f40d2460 100644
--- a/librdmacm/src/rsocket.c
+++ b/librdmacm/src/rsocket.c
@@ -404,6 +404,20 @@ struct ds_udp_header {

 #define ds_next_qp(qp) container_of((qp)->list.next, struct ds_qp, list)

+static void write_all(int fd, const void *msg, size_t len)
+{
+	// FIXME: if fd is a socket this really needs to handle EINTR and other conditions.
+	ssize_t rc = write(fd, msg, len);
+	assert(rc == len);
+}
+
+static void read_all(int fd, void *msg, size_t len)
+{
+	// FIXME: if fd is a socket this really needs to handle EINTR and other conditions.
+	ssize_t rc = read(fd, msg, len);
+	assert(rc == len);
+}
+
 static void ds_insert_qp(struct rsocket *rs, struct ds_qp *qp)
 {
 	if (!rs->qp_list)
@@ -444,8 +458,8 @@ static int rs_notify_svc(struct rs_svc *svc, struct rsocket *rs, int cmd)
 	msg.cmd = cmd;
 	msg.status = EINVAL;
 	msg.rs = rs;
-	write(svc->sock[0], &msg, sizeof msg);
-	read(svc->sock[0], &msg, sizeof msg);
+	write_all(svc->sock[0], &msg, sizeof msg);
+	read_all(svc->sock[0], &msg, sizeof msg);
 	ret = rdma_seterrno(msg.status);
 	if (svc->cnt)
 		goto unlock;
@@ -484,6 +498,15 @@ static int rs_scale_to_value(int value, int bits)
 	       value : (value & ~(1 << (bits - 1))) << bits;
 }

+/* gcc > ~5 will not allow (void)fscanf to suppress -Wunused-result, but this
+   will do it.  In this case ignoring the result is OK (but horribly
+   unfriendly to user) since the library has a sane default. */
+#define failable_fscanf(f, fmt, ...)                                           \
+	{                                                                      \
+		int rc = fscanf(f, fmt, __VA_ARGS__);                          \
+		(void) rc;                                                     \
+	}
+
 void rs_configure(void)
 {
 	FILE *f;
@@ -501,27 +524,27 @@ void rs_configure(void)
 	ucma_ib_init();

 	if ((f = fopen(RS_CONF_DIR "/polling_time", "r"))) {
-		(void) fscanf(f, "%u", &polling_time);
+		failable_fscanf(f, "%u", &polling_time);
 		fclose(f);
 	}

 	if ((f = fopen(RS_CONF_DIR "/inline_default", "r"))) {
-		(void) fscanf(f, "%hu", &def_inline);
+		failable_fscanf(f, "%hu", &def_inline);
 		fclose(f);
 	}

 	if ((f = fopen(RS_CONF_DIR "/sqsize_default", "r"))) {
-		(void) fscanf(f, "%hu", &def_sqsize);
+		failable_fscanf(f, "%hu", &def_sqsize);
 		fclose(f);
 	}

 	if ((f = fopen(RS_CONF_DIR "/rqsize_default", "r"))) {
-		(void) fscanf(f, "%hu", &def_rqsize);
+		failable_fscanf(f, "%hu", &def_rqsize);
 		fclose(f);
 	}

 	if ((f = fopen(RS_CONF_DIR "/mem_default", "r"))) {
-		(void) fscanf(f, "%u", &def_mem);
+		failable_fscanf(f, "%u", &def_mem);
 		fclose(f);

 		if (def_mem < 1)
@@ -529,14 +552,14 @@ void rs_configure(void)
 	}

 	if ((f = fopen(RS_CONF_DIR "/wmem_default", "r"))) {
-		(void) fscanf(f, "%u", &def_wmem);
+		failable_fscanf(f, "%u", &def_wmem);
 		fclose(f);
 		if (def_wmem < RS_SNDLOWAT)
 			def_wmem = RS_SNDLOWAT << 1;
 	}

 	if ((f = fopen(RS_CONF_DIR "/iomap_size", "r"))) {
-		(void) fscanf(f, "%hu", &def_iomap_size);
+		failable_fscanf(f, "%hu", &def_iomap_size);
 		fclose(f);

 		/* round to supported values */
@@ -3345,7 +3368,8 @@ static int rs_set_keepalive(struct rsocket *rs, int on)
 	if (on) {
 		if (!rs->keepalive_time) {
 			if ((f = fopen("/proc/sys/net/ipv4/tcp_keepalive_time", "r"))) {
-				(void) fscanf(f, "%u", &rs->keepalive_time);
+				if (fscanf(f, "%u", &rs->keepalive_time) != 1)
+					rs->keepalive_time = 7200;
 				fclose(f);
 			} else {
 				rs->keepalive_time = 7200;
@@ -3985,7 +4009,7 @@ static void udp_svc_process_sock(struct rs_svc *svc)
 {
 	struct rs_svc_msg msg;

-	read(svc->sock[1], &msg, sizeof msg);
+	read_all(svc->sock[1], &msg, sizeof msg);
 	switch (msg.cmd) {
 	case RS_SVC_ADD_DGRAM:
 		msg.status = rs_svc_add_rs(svc, msg.rs);
@@ -4009,7 +4033,7 @@ static void udp_svc_process_sock(struct rs_svc *svc)
 		break;
 	}

-	write(svc->sock[1], &msg, sizeof msg);
+	write_all(svc->sock[1], &msg, sizeof msg);
 }

 static uint8_t udp_svc_sgid_index(struct ds_dest *dest, union ibv_gid *sgid)
@@ -4184,7 +4208,7 @@ static void *udp_svc_run(void *arg)
 	ret = rs_svc_grow_sets(svc, 4);
 	if (ret) {
 		msg.status = ret;
-		write(svc->sock[1], &msg, sizeof msg);
+		write_all(svc->sock[1], &msg, sizeof msg);
 		return (void *) (uintptr_t) ret;
 	}

@@ -4222,7 +4246,7 @@ static void tcp_svc_process_sock(struct rs_svc *svc)
 	struct rs_svc_msg msg;
 	int i;

-	read(svc->sock[1], &msg, sizeof msg);
+	read_all(svc->sock[1], &msg, sizeof msg);
 	switch (msg.cmd) {
 	case RS_SVC_ADD_KEEPALIVE:
 		msg.status = rs_svc_add_rs(svc, msg.rs);
@@ -4253,7 +4277,7 @@ static void tcp_svc_process_sock(struct rs_svc *svc)
 	default:
 		break;
 	}
-	write(svc->sock[1], &msg, sizeof msg);
+	write_all(svc->sock[1], &msg, sizeof msg);
 }

 /*
@@ -4282,7 +4306,7 @@ static void *tcp_svc_run(void *arg)
 	ret = rs_svc_grow_sets(svc, 16);
 	if (ret) {
 		msg.status = ret;
-		write(svc->sock[1], &msg, sizeof msg);
+		write_all(svc->sock[1], &msg, sizeof msg);
 		return (void *) (uintptr_t) ret;
 	}



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html






