Message-ID: <yo2qj7psn3sqtyqgsfn6y2qtwcmyb4j7gwuffg34gwqwkrsyox@4aff3wvdrdgu>
Date: Thu, 14 Nov 2024 11:28:44 +0100
From: Stefano Garzarella <sgarzare@...hat.com>
To: Konstantin Shkolnyy <kshk@...ux.ibm.com>
Cc: virtualization@...ts.linux.dev, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, mjrosato@...ux.ibm.com
Subject: Re: [PATCH v6 3/3] vsock/test: verify socket options after setting
 them

On Wed, Nov 13, 2024 at 08:35:57AM -0600, Konstantin Shkolnyy wrote:
>Replace setsockopt() calls with calls to functions that follow
>setsockopt() with getsockopt() and check that the returned value and its
>size are the same as have been set. (Except in vsock_perf.)
>
>Signed-off-by: Konstantin Shkolnyy <kshk@...ux.ibm.com>
>---
> tools/testing/vsock/control.c             |   9 +-
> tools/testing/vsock/msg_zerocopy_common.c |  10 --
> tools/testing/vsock/msg_zerocopy_common.h |   1 -
> tools/testing/vsock/util.c                | 144 ++++++++++++++++++++++
> tools/testing/vsock/util.h                |   7 ++
> tools/testing/vsock/vsock_perf.c          |  10 ++
> tools/testing/vsock/vsock_test.c          |  49 +++-----
> tools/testing/vsock/vsock_test_zerocopy.c |   2 +-
> tools/testing/vsock/vsock_uring_test.c    |   2 +-
> 9 files changed, 181 insertions(+), 53 deletions(-)
>
>diff --git a/tools/testing/vsock/control.c b/tools/testing/vsock/control.c
>index d2deb4b15b94..875ef0cfa415 100644
>--- a/tools/testing/vsock/control.c
>+++ b/tools/testing/vsock/control.c
>@@ -27,6 +27,7 @@
>
> #include "timeout.h"
> #include "control.h"
>+#include "util.h"
>
> static int control_fd = -1;
>
>@@ -50,7 +51,6 @@ void control_init(const char *control_host,
>
> 	for (ai = result; ai; ai = ai->ai_next) {
> 		int fd;
>-		int val = 1;
>
> 		fd = socket(ai->ai_family, ai->ai_socktype, ai->ai_protocol);
> 		if (fd < 0)
>@@ -65,11 +65,8 @@ void control_init(const char *control_host,
> 			break;
> 		}
>
>-		if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
>-			       &val, sizeof(val)) < 0) {
>-			perror("setsockopt");
>-			exit(EXIT_FAILURE);
>-		}
>+		setsockopt_int_check(fd, SOL_SOCKET, SO_REUSEADDR, 1,
>+				"setsockopt SO_REUSEADDR");
>
> 		if (bind(fd, ai->ai_addr, ai->ai_addrlen) < 0)
> 			goto next;
>diff --git a/tools/testing/vsock/msg_zerocopy_common.c b/tools/testing/vsock/msg_zerocopy_common.c
>index 5a4bdf7b5132..8622e5a0f8b7 100644
>--- a/tools/testing/vsock/msg_zerocopy_common.c
>+++ b/tools/testing/vsock/msg_zerocopy_common.c
>@@ -14,16 +14,6 @@
>
> #include "msg_zerocopy_common.h"
>
>-void enable_so_zerocopy(int fd)
>-{
>-	int val = 1;
>-
>-	if (setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, &val, sizeof(val))) {
>-		perror("setsockopt");
>-		exit(EXIT_FAILURE);
>-	}
>-}
>-

Since the new API has a different name (i.e.
`enable_so_zerocopy_check()`), this `enable_so_zerocopy()` could stay
here, anyway I don't want to be too picky, I'm totally fine with this
change since it's now only used by vsock_perf ;-)

Reviewed-by: Stefano Garzarella <sgarzare@...hat.com>

Thanks,
Stefano

> void vsock_recv_completion(int fd, const bool *zerocopied)
> {
> 	struct sock_extended_err *serr;
>diff --git a/tools/testing/vsock/msg_zerocopy_common.h b/tools/testing/vsock/msg_zerocopy_common.h
>index 3763c5ccedb9..ad14139e93ca 100644
>--- a/tools/testing/vsock/msg_zerocopy_common.h
>+++ b/tools/testing/vsock/msg_zerocopy_common.h
>@@ -12,7 +12,6 @@
> #define VSOCK_RECVERR	1
> #endif
>
>-void enable_so_zerocopy(int fd);
> void vsock_recv_completion(int fd, const bool *zerocopied);
>
> #endif /* MSG_ZEROCOPY_COMMON_H */
>diff --git a/tools/testing/vsock/util.c b/tools/testing/vsock/util.c
>index a3d448a075e3..e79534b52477 100644
>--- a/tools/testing/vsock/util.c
>+++ b/tools/testing/vsock/util.c
>@@ -651,3 +651,147 @@ void free_test_iovec(const struct iovec *test_iovec,
>
> 	free(iovec);
> }
>+
>+/* Set "unsigned long long" socket option and check that it's indeed set */
>+void setsockopt_ull_check(int fd, int level, int optname,
>+	unsigned long long val, char const *errmsg)
>+{
>+	unsigned long long chkval;
>+	socklen_t chklen;
>+	int err;
>+
>+	err = setsockopt(fd, level, optname, &val, sizeof(val));
>+	if (err) {
>+		fprintf(stderr, "setsockopt err: %s (%d)\n",
>+				strerror(errno), errno);
>+		goto fail;
>+	}
>+
>+	chkval = ~val; /* just make storage != val */
>+	chklen = sizeof(chkval);
>+
>+	err = getsockopt(fd, level, optname, &chkval, &chklen);
>+	if (err) {
>+		fprintf(stderr, "getsockopt err: %s (%d)\n",
>+				strerror(errno), errno);
>+		goto fail;
>+	}
>+
>+	if (chklen != sizeof(chkval)) {
>+		fprintf(stderr, "size mismatch: set %zu got %d\n", sizeof(val),
>+				chklen);
>+		goto fail;
>+	}
>+
>+	if (chkval != val) {
>+		fprintf(stderr, "value mismatch: set %llu got %llu\n", val,
>+				chkval);
>+		goto fail;
>+	}
>+	return;
>+fail:
>+	fprintf(stderr, "%s  val %llu\n", errmsg, val);
>+	exit(EXIT_FAILURE);
>+;
>+}
>+
>+/* Set "int" socket option and check that it's indeed set */
>+void setsockopt_int_check(int fd, int level, int optname, int val,
>+		char const *errmsg)
>+{
>+	int chkval;
>+	socklen_t chklen;
>+	int err;
>+
>+	err = setsockopt(fd, level, optname, &val, sizeof(val));
>+	if (err) {
>+		fprintf(stderr, "setsockopt err: %s (%d)\n",
>+				strerror(errno), errno);
>+		goto fail;
>+	}
>+
>+	chkval = ~val; /* just make storage != val */
>+	chklen = sizeof(chkval);
>+
>+	err = getsockopt(fd, level, optname, &chkval, &chklen);
>+	if (err) {
>+		fprintf(stderr, "getsockopt err: %s (%d)\n",
>+				strerror(errno), errno);
>+		goto fail;
>+	}
>+
>+	if (chklen != sizeof(chkval)) {
>+		fprintf(stderr, "size mismatch: set %zu got %d\n", sizeof(val),
>+				chklen);
>+		goto fail;
>+	}
>+
>+	if (chkval != val) {
>+		fprintf(stderr, "value mismatch: set %d got %d\n",
>+				val, chkval);
>+		goto fail;
>+	}
>+	return;
>+fail:
>+	fprintf(stderr, "%s val %d\n", errmsg, val);
>+	exit(EXIT_FAILURE);
>+}
>+
>+static void mem_invert(unsigned char *mem, size_t size)
>+{
>+	size_t i;
>+
>+	for (i = 0; i < size; i++)
>+		mem[i] = ~mem[i];
>+}
>+
>+/* Set "timeval" socket option and check that it's indeed set */
>+void setsockopt_timeval_check(int fd, int level, int optname,
>+		struct timeval val, char const *errmsg)
>+{
>+	struct timeval chkval;
>+	socklen_t chklen;
>+	int err;
>+
>+	err = setsockopt(fd, level, optname, &val, sizeof(val));
>+	if (err) {
>+		fprintf(stderr, "setsockopt err: %s (%d)\n",
>+				strerror(errno), errno);
>+		goto fail;
>+	}
>+
>+	 /* just make storage != val */
>+	chkval = val;
>+	mem_invert((unsigned char *) &chkval, sizeof(chkval));
>+	chklen = sizeof(chkval);
>+
>+	err = getsockopt(fd, level, optname, &chkval, &chklen);
>+	if (err) {
>+		fprintf(stderr, "getsockopt err: %s (%d)\n",
>+				strerror(errno), errno);
>+		goto fail;
>+	}
>+
>+	if (chklen != sizeof(chkval)) {
>+		fprintf(stderr, "size mismatch: set %zu got %d\n", sizeof(val),
>+				chklen);
>+		goto fail;
>+	}
>+
>+	if (memcmp(&chkval, &val, sizeof(val)) != 0) {
>+		fprintf(stderr, "value mismatch: set %ld:%ld got %ld:%ld\n",
>+				val.tv_sec, val.tv_usec,
>+				chkval.tv_sec, chkval.tv_usec);
>+		goto fail;
>+	}
>+	return;
>+fail:
>+	fprintf(stderr, "%s val %ld:%ld\n", errmsg, val.tv_sec, val.tv_usec);
>+	exit(EXIT_FAILURE);
>+}
>+
>+void enable_so_zerocopy_check(int fd)
>+{
>+	setsockopt_int_check(fd, SOL_SOCKET, SO_ZEROCOPY, 1,
>+			"setsockopt SO_ZEROCOPY");
>+}
>diff --git a/tools/testing/vsock/util.h b/tools/testing/vsock/util.h
>index fff22d4a14c0..f189334591df 100644
>--- a/tools/testing/vsock/util.h
>+++ b/tools/testing/vsock/util.h
>@@ -68,4 +68,11 @@ unsigned long iovec_hash_djb2(const struct iovec *iov, size_t iovnum);
> struct iovec *alloc_test_iovec(const struct iovec *test_iovec, int iovnum);
> void free_test_iovec(const struct iovec *test_iovec,
> 		     struct iovec *iovec, int iovnum);
>+void setsockopt_ull_check(int fd, int level, int optname,
>+		unsigned long long val, char const *errmsg);
>+void setsockopt_int_check(int fd, int level, int optname, int val,
>+		char const *errmsg);
>+void setsockopt_timeval_check(int fd, int level, int optname,
>+		struct timeval val, char const *errmsg);
>+void enable_so_zerocopy_check(int fd);
> #endif /* UTIL_H */
>diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c
>index 8e0a6c0770d3..75971ac708c9 100644
>--- a/tools/testing/vsock/vsock_perf.c
>+++ b/tools/testing/vsock/vsock_perf.c
>@@ -251,6 +251,16 @@ static void run_receiver(int rcvlowat_bytes)
> 	close(fd);
> }
>
>+static void enable_so_zerocopy(int fd)
>+{
>+	int val = 1;
>+
>+	if (setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, &val, sizeof(val))) {
>+		perror("setsockopt");
>+		exit(EXIT_FAILURE);
>+	}
>+}
>+
> static void run_sender(int peer_cid, unsigned long to_send_bytes)
> {
> 	time_t tx_begin_ns;
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index c7af23332e57..0b514643a9a5 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -444,17 +444,12 @@ static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
>
> 	sock_buf_size = SOCK_BUF_SIZE;
>
>-	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>-		       &sock_buf_size, sizeof(sock_buf_size))) {
>-		perror("setsockopt(SO_VM_SOCKETS_BUFFER_MAX_SIZE)");
>-		exit(EXIT_FAILURE);
>-	}
>+	setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>+			sock_buf_size,
>+			"setsockopt(SO_VM_SOCKETS_BUFFER_MAX_SIZE)");
>
>-	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>-		       &sock_buf_size, sizeof(sock_buf_size))) {
>-		perror("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
>-		exit(EXIT_FAILURE);
>-	}
>+	setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>+		       sock_buf_size, "setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
>
> 	/* Ready to receive data. */
> 	control_writeln("SRVREADY");
>@@ -586,10 +581,8 @@ static void test_seqpacket_timeout_client(const struct test_opts *opts)
> 	tv.tv_sec = RCVTIMEO_TIMEOUT_SEC;
> 	tv.tv_usec = 0;
>
>-	if (setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, (void *)&tv, sizeof(tv)) == -1) {
>-		perror("setsockopt(SO_RCVTIMEO)");
>-		exit(EXIT_FAILURE);
>-	}
>+	setsockopt_timeval_check(fd, SOL_SOCKET, SO_RCVTIMEO, tv,
>+			"setsockopt(SO_RCVTIMEO)");
>
> 	read_enter_ns = current_nsec();
>
>@@ -855,11 +848,8 @@ static void test_stream_poll_rcvlowat_client(const struct test_opts *opts)
> 		exit(EXIT_FAILURE);
> 	}
>
>-	if (setsockopt(fd, SOL_SOCKET, SO_RCVLOWAT,
>-		       &lowat_val, sizeof(lowat_val))) {
>-		perror("setsockopt(SO_RCVLOWAT)");
>-		exit(EXIT_FAILURE);
>-	}
>+	setsockopt_int_check(fd, SOL_SOCKET, SO_RCVLOWAT,
>+		       lowat_val, "setsockopt(SO_RCVLOWAT)");
>
> 	control_expectln("SRVSENT");
>
>@@ -1383,11 +1373,8 @@ static void test_stream_credit_update_test(const struct test_opts *opts,
> 	/* size_t can be < unsigned long long */
> 	sock_buf_size = buf_size;
>
>-	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>-		       &sock_buf_size, sizeof(sock_buf_size))) {
>-		perror("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
>-		exit(EXIT_FAILURE);
>-	}
>+	setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>+		       sock_buf_size, "setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
>
> 	if (low_rx_bytes_test) {
> 		/* Set new SO_RCVLOWAT here. This enables sending credit
>@@ -1396,11 +1383,8 @@ static void test_stream_credit_update_test(const struct test_opts *opts,
> 		 */
> 		recv_buf_size = 1 + VIRTIO_VSOCK_MAX_PKT_BUF_SIZE;
>
>-		if (setsockopt(fd, SOL_SOCKET, SO_RCVLOWAT,
>-			       &recv_buf_size, sizeof(recv_buf_size))) {
>-			perror("setsockopt(SO_RCVLOWAT)");
>-			exit(EXIT_FAILURE);
>-		}
>+		setsockopt_int_check(fd, SOL_SOCKET, SO_RCVLOWAT,
>+			       recv_buf_size, "setsockopt(SO_RCVLOWAT)");
> 	}
>
> 	/* Send one dummy byte here, because 'setsockopt()' above also
>@@ -1442,11 +1426,8 @@ static void test_stream_credit_update_test(const struct test_opts *opts,
> 		recv_buf_size++;
>
> 		/* Updating SO_RCVLOWAT will send credit update. */
>-		if (setsockopt(fd, SOL_SOCKET, SO_RCVLOWAT,
>-			       &recv_buf_size, sizeof(recv_buf_size))) {
>-			perror("setsockopt(SO_RCVLOWAT)");
>-			exit(EXIT_FAILURE);
>-		}
>+		setsockopt_int_check(fd, SOL_SOCKET, SO_RCVLOWAT,
>+			       recv_buf_size, "setsockopt(SO_RCVLOWAT)");
> 	}
>
> 	fds.fd = fd;
>diff --git a/tools/testing/vsock/vsock_test_zerocopy.c b/tools/testing/vsock/vsock_test_zerocopy.c
>index 04c376b6937f..9d9a6cb9614a 100644
>--- a/tools/testing/vsock/vsock_test_zerocopy.c
>+++ b/tools/testing/vsock/vsock_test_zerocopy.c
>@@ -162,7 +162,7 @@ static void test_client(const struct test_opts *opts,
> 	}
>
> 	if (test_data->so_zerocopy)
>-		enable_so_zerocopy(fd);
>+		enable_so_zerocopy_check(fd);
>
> 	iovec = alloc_test_iovec(test_data->vecs, test_data->vecs_cnt);
>
>diff --git a/tools/testing/vsock/vsock_uring_test.c b/tools/testing/vsock/vsock_uring_test.c
>index 6c3e6f70c457..5c3078969659 100644
>--- a/tools/testing/vsock/vsock_uring_test.c
>+++ b/tools/testing/vsock/vsock_uring_test.c
>@@ -73,7 +73,7 @@ static void vsock_io_uring_client(const struct test_opts *opts,
> 	}
>
> 	if (msg_zerocopy)
>-		enable_so_zerocopy(fd);
>+		enable_so_zerocopy_check(fd);
>
> 	iovec = alloc_test_iovec(test_data->vecs, test_data->vecs_cnt);
>
>-- 
>2.34.1
>

Powered by blists - more mailing lists