[PATCH v5 0/3] vsock/test: fix wrong setsockopt() parameters

[PATCH v5 0/3] vsock/test: fix wrong setsockopt() parameters

[Konstantin Shkolnyy]

Parameters were created using wrong C types, which caused them to be of
wrong size on some architectures, causing problems.

The problem with SO_RCVLOWAT was found on s390 (big endian), while x86-64
didn't show it. After the fix, all tests pass on s390.
Then Stefano Garzarella pointed out that SO_VM_SOCKETS_* calls might have
a similar problem, which turned out to be true, hence, the second patch.

Changes for v5:
- in the patch #2 replace the introduced uint64_t with unsigned long long
to match documentation
- add a patch #3 that verifies every setsockopt() call.
Changes for v4:
- add "Reviewed-by:" to the first patch, and add a second patch fixing
SO_VM_SOCKETS_* calls, which depends on the first one (hence, it's now
a patch series.)
Changes for v3:
- fix the same problem in vsock_perf and update commit message
Changes for v2:
- add "Fixes:" lines to the commit message

Konstantin Shkolnyy (3):
  vsock/test: fix failures due to wrong SO_RCVLOWAT parameter
  vsock/test: fix parameter types in SO_VM_SOCKETS_* calls
  vsock/test: verify socket options after setting them

 tools/testing/vsock/Makefile              |   8 +-
 tools/testing/vsock/control.c             |   8 +-
 tools/testing/vsock/msg_zerocopy_common.c |   8 +-
 tools/testing/vsock/util_socket.c         | 149 ++++++++++++++++++++++
 tools/testing/vsock/util_socket.h         |  19 +++
 tools/testing/vsock/vsock_perf.c          |  34 ++---
 tools/testing/vsock/vsock_test.c          |  64 +++++-----
 7 files changed, 231 insertions(+), 59 deletions(-)
 create mode 100644 tools/testing/vsock/util_socket.c
 create mode 100644 tools/testing/vsock/util_socket.h

-- 
2.34.1

[PATCH v5 1/3] vsock/test: fix failures due to wrong SO_RCVLOWAT parameter

[Konstantin Shkolnyy]

This happens on 64-bit big-endian machines.
SO_RCVLOWAT requires an int parameter. However, instead of int, the test
uses unsigned long in one place and size_t in another. Both are 8 bytes
long on 64-bit machines. The kernel, having received the 8 bytes, doesn't
test for the exact size of the parameter, it only cares that it's >=
sizeof(int), and casts the 4 lower-addressed bytes to an int, which, on
a big-endian machine, contains 0. 0 doesn't trigger an error, SO_RCVLOWAT
returns with success and the socket stays with the default SO_RCVLOWAT = 1,
which results in vsock_test failures, while vsock_perf doesn't even notice
that it's failed to change it.

Fixes: b1346338fbae ("vsock_test: POLLIN + SO_RCVLOWAT test")
Fixes: 542e893fbadc ("vsock/test: two tests to check credit update logic")
Fixes: 8abbffd27ced ("test/vsock: vsock_perf utility")
Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
---
 tools/testing/vsock/vsock_perf.c | 6 +++---
 tools/testing/vsock/vsock_test.c | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c
index 4e8578f815e0..22633c2848cc 100644
--- a/tools/testing/vsock/vsock_perf.c
+++ b/tools/testing/vsock/vsock_perf.c
@@ -133,7 +133,7 @@ static float get_gbps(unsigned long bits, time_t ns_delta)
 	       ((float)ns_delta / NSEC_PER_SEC);
 }
 
-static void run_receiver(unsigned long rcvlowat_bytes)
+static void run_receiver(int rcvlowat_bytes)
 {
 	unsigned int read_cnt;
 	time_t rx_begin_ns;
@@ -163,7 +163,7 @@ static void run_receiver(unsigned long rcvlowat_bytes)
 	printf("Listen port %u\n", port);
 	printf("RX buffer %lu bytes\n", buf_size_bytes);
 	printf("vsock buffer %lu bytes\n", vsock_buf_bytes);
-	printf("SO_RCVLOWAT %lu bytes\n", rcvlowat_bytes);
+	printf("SO_RCVLOWAT %d bytes\n", rcvlowat_bytes);
 
 	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
 
@@ -439,7 +439,7 @@ static long strtolx(const char *arg)
 int main(int argc, char **argv)
 {
 	unsigned long to_send_bytes = DEFAULT_TO_SEND_BYTES;
-	unsigned long rcvlowat_bytes = DEFAULT_RCVLOWAT_BYTES;
+	int rcvlowat_bytes = DEFAULT_RCVLOWAT_BYTES;
 	int peer_cid = -1;
 	bool sender = false;
 
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 8d38dbf8f41f..7fd25b814b4b 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -835,7 +835,7 @@ static void test_stream_poll_rcvlowat_server(const struct test_opts *opts)
 
 static void test_stream_poll_rcvlowat_client(const struct test_opts *opts)
 {
-	unsigned long lowat_val = RCVLOWAT_BUF_SIZE;
+	int lowat_val = RCVLOWAT_BUF_SIZE;
 	char buf[RCVLOWAT_BUF_SIZE];
 	struct pollfd fds;
 	short poll_flags;
@@ -1357,7 +1357,7 @@ static void test_stream_rcvlowat_def_cred_upd_client(const struct test_opts *opt
 static void test_stream_credit_update_test(const struct test_opts *opts,
 					   bool low_rx_bytes_test)
 {
-	size_t recv_buf_size;
+	int recv_buf_size;
 	struct pollfd fds;
 	size_t buf_size;
 	void *buf;
-- 
2.34.1

[PATCH v5 2/3] vsock/test: fix parameter types in SO_VM_SOCKETS_* calls

[Konstantin Shkolnyy]

Change parameters of SO_VM_SOCKETS_* to unsigned long long as documented
in the vm_sockets.h, because the corresponding kernel code requires them
to be at least 64-bit, no matter what architecture. Otherwise they are
too small on 32-bit machines.

Fixes: 5c338112e48a ("test/vsock: rework message bounds test")
Fixes: 685a21c314a8 ("test/vsock: add big message test")
Fixes: 542e893fbadc ("vsock/test: two tests to check credit update logic")
Fixes: 8abbffd27ced ("test/vsock: vsock_perf utility")
Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
---
 tools/testing/vsock/vsock_perf.c |  4 ++--
 tools/testing/vsock/vsock_test.c | 22 +++++++++++++++++-----
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c
index 22633c2848cc..8e0a6c0770d3 100644
--- a/tools/testing/vsock/vsock_perf.c
+++ b/tools/testing/vsock/vsock_perf.c
@@ -33,7 +33,7 @@
 
 static unsigned int port = DEFAULT_PORT;
 static unsigned long buf_size_bytes = DEFAULT_BUF_SIZE_BYTES;
-static unsigned long vsock_buf_bytes = DEFAULT_VSOCK_BUF_BYTES;
+static unsigned long long vsock_buf_bytes = DEFAULT_VSOCK_BUF_BYTES;
 static bool zerocopy;
 
 static void error(const char *s)
@@ -162,7 +162,7 @@ static void run_receiver(int rcvlowat_bytes)
 	printf("Run as receiver\n");
 	printf("Listen port %u\n", port);
 	printf("RX buffer %lu bytes\n", buf_size_bytes);
-	printf("vsock buffer %lu bytes\n", vsock_buf_bytes);
+	printf("vsock buffer %llu bytes\n", vsock_buf_bytes);
 	printf("SO_RCVLOWAT %d bytes\n", rcvlowat_bytes);
 
 	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index 7fd25b814b4b..c7af23332e57 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -429,7 +429,7 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
 
 static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
 {
-	unsigned long sock_buf_size;
+	unsigned long long sock_buf_size;
 	unsigned long remote_hash;
 	unsigned long curr_hash;
 	int fd;
@@ -634,7 +634,8 @@ static void test_seqpacket_timeout_server(const struct test_opts *opts)
 
 static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
 {
-	unsigned long sock_buf_size;
+	unsigned long long sock_buf_size;
+	size_t buf_size;
 	socklen_t len;
 	void *data;
 	int fd;
@@ -655,13 +656,20 @@ static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
 
 	sock_buf_size++;
 
-	data = malloc(sock_buf_size);
+	/* size_t can be < unsigned long long */
+	buf_size = (size_t) sock_buf_size;
+	if (buf_size != sock_buf_size) {
+		fprintf(stderr, "Returned BUFFER_SIZE too large\n");
+		exit(EXIT_FAILURE);
+	}
+
+	data = malloc(buf_size);
 	if (!data) {
 		perror("malloc");
 		exit(EXIT_FAILURE);
 	}
 
-	send_buf(fd, data, sock_buf_size, 0, -EMSGSIZE);
+	send_buf(fd, data, buf_size, 0, -EMSGSIZE);
 
 	control_writeln("CLISENT");
 
@@ -1360,6 +1368,7 @@ static void test_stream_credit_update_test(const struct test_opts *opts,
 	int recv_buf_size;
 	struct pollfd fds;
 	size_t buf_size;
+	unsigned long long sock_buf_size;
 	void *buf;
 	int fd;
 
@@ -1371,8 +1380,11 @@ static void test_stream_credit_update_test(const struct test_opts *opts,
 
 	buf_size = RCVLOWAT_CREDIT_UPD_BUF_SIZE;
 
+	/* size_t can be < unsigned long long */
+	sock_buf_size = buf_size;
+
 	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
-		       &buf_size, sizeof(buf_size))) {
+		       &sock_buf_size, sizeof(sock_buf_size))) {
 		perror("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
 		exit(EXIT_FAILURE);
 	}
-- 
2.34.1

[PATCH v5 3/3] vsock/test: verify socket options after setting them

[Konstantin Shkolnyy]

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.

Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
---
 tools/testing/vsock/Makefile              |   8 +-
 tools/testing/vsock/control.c             |   8 +-
 tools/testing/vsock/msg_zerocopy_common.c |   8 +-
 tools/testing/vsock/util_socket.c         | 149 ++++++++++++++++++++++
 tools/testing/vsock/util_socket.h         |  19 +++
 tools/testing/vsock/vsock_perf.c          |  24 ++--
 tools/testing/vsock/vsock_test.c          |  40 +++---
 7 files changed, 208 insertions(+), 48 deletions(-)
 create mode 100644 tools/testing/vsock/util_socket.c
 create mode 100644 tools/testing/vsock/util_socket.h

diff --git a/tools/testing/vsock/Makefile b/tools/testing/vsock/Makefile
index 6e0b4e95e230..1ec0b3a67aa4 100644
--- a/tools/testing/vsock/Makefile
+++ b/tools/testing/vsock/Makefile
@@ -1,12 +1,12 @@
 # SPDX-License-Identifier: GPL-2.0-only
 all: test vsock_perf
 test: vsock_test vsock_diag_test vsock_uring_test
-vsock_test: vsock_test.o vsock_test_zerocopy.o timeout.o control.o util.o msg_zerocopy_common.o
-vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o
-vsock_perf: vsock_perf.o msg_zerocopy_common.o
+vsock_test: vsock_test.o vsock_test_zerocopy.o timeout.o control.o util.o msg_zerocopy_common.o util_socket.o
+vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o util_socket.o
+vsock_perf: vsock_perf.o msg_zerocopy_common.o util_socket.o
 
 vsock_uring_test: LDLIBS = -luring
-vsock_uring_test: control.o util.o vsock_uring_test.o timeout.o msg_zerocopy_common.o
+vsock_uring_test: control.o util.o vsock_uring_test.o timeout.o msg_zerocopy_common.o util_socket.o
 
 CFLAGS += -g -O2 -Werror -Wall -I. -I../../include -I../../../usr/include -Wno-pointer-sign -fno-strict-overflow -fno-strict-aliasing -fno-common -MMD -U_FORTIFY_SOURCE -D_GNU_SOURCE
 .PHONY: all test clean
diff --git a/tools/testing/vsock/control.c b/tools/testing/vsock/control.c
index d2deb4b15b94..f1fd809ac9d5 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_socket.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,9 @@ void control_init(const char *control_host,
 			break;
 		}
 
-		if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
-			       &val, sizeof(val)) < 0) {
-			perror("setsockopt");
+		if (!setsockopt_int_check(fd, SOL_SOCKET, SO_REUSEADDR, 1,
+				"setsockopt SO_REUSEADDR"))
 			exit(EXIT_FAILURE);
-		}
 
 		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..4edb1b6974c0 100644
--- a/tools/testing/vsock/msg_zerocopy_common.c
+++ b/tools/testing/vsock/msg_zerocopy_common.c
@@ -13,15 +13,13 @@
 #include <linux/errqueue.h>
 
 #include "msg_zerocopy_common.h"
+#include "util_socket.h"
 
 void enable_so_zerocopy(int fd)
 {
-	int val = 1;
-
-	if (setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, &val, sizeof(val))) {
-		perror("setsockopt");
+	if (!setsockopt_int_check(fd, SOL_SOCKET, SO_ZEROCOPY, 1,
+			"setsockopt SO_ZEROCOPY"))
 		exit(EXIT_FAILURE);
-	}
 }
 
 void vsock_recv_completion(int fd, const bool *zerocopied)
diff --git a/tools/testing/vsock/util_socket.c b/tools/testing/vsock/util_socket.c
new file mode 100644
index 000000000000..e791da160624
--- /dev/null
+++ b/tools/testing/vsock/util_socket.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Socket utility functions.
+ *
+ * Copyright IBM Corp. 2024
+ */
+#include <errno.h>
+#include <inttypes.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/socket.h>
+#include "util_socket.h"
+
+/* Set "unsigned long long" socket option and check that it's indeed set */
+bool 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 true;
+fail:
+	fprintf(stderr, "%s  val %llu\n", errmsg, val);
+	return false;
+}
+
+/* Set "int" socket option and check that it's indeed set */
+bool 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 true;
+fail:
+	fprintf(stderr, "%s val %d\n", errmsg, val);
+	return false;
+}
+
+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 */
+bool 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 true;
+fail:
+	fprintf(stderr, "%s val %ld:%ld\n", errmsg, val.tv_sec, val.tv_usec);
+	return false;
+}
diff --git a/tools/testing/vsock/util_socket.h b/tools/testing/vsock/util_socket.h
new file mode 100644
index 000000000000..38cf3decb15c
--- /dev/null
+++ b/tools/testing/vsock/util_socket.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Socket utility functions.
+ *
+ * Copyright IBM Corp. 2024
+ */
+#ifndef UTIL_SOCKET_H
+#define UTIL_SOCKET_H
+
+#include <stdbool.h>
+
+bool setsockopt_ull_check(int fd, int level, int optname,
+		unsigned long long val, char const *errmsg);
+bool setsockopt_int_check(int fd, int level, int optname, int val,
+		char const *errmsg);
+bool setsockopt_timeval_check(int fd, int level, int optname,
+		struct timeval val, char const *errmsg);
+
+#endif /* UTIL_SOCKET_H */
diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c
index 8e0a6c0770d3..b117e043b87b 100644
--- a/tools/testing/vsock/vsock_perf.c
+++ b/tools/testing/vsock/vsock_perf.c
@@ -21,6 +21,7 @@
 #include <sys/mman.h>
 
 #include "msg_zerocopy_common.h"
+#include "util_socket.h"
 
 #define DEFAULT_BUF_SIZE_BYTES	(128 * 1024)
 #define DEFAULT_TO_SEND_BYTES	(64 * 1024)
@@ -88,13 +89,16 @@ static unsigned long memparse(const char *ptr)
 
 static void vsock_increase_buf_size(int fd)
 {
-	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
-		       &vsock_buf_bytes, sizeof(vsock_buf_bytes)))
-		error("setsockopt(SO_VM_SOCKETS_BUFFER_MAX_SIZE)");
+	if (!setsockopt_ull_check(fd, AF_VSOCK,
+			SO_VM_SOCKETS_BUFFER_MAX_SIZE, vsock_buf_bytes,
+			"setsockopt(SO_VM_SOCKETS_BUFFER_MAX_SIZE)"))
+		exit(EXIT_FAILURE);
+
+	if (!setsockopt_ull_check(fd, AF_VSOCK,
+			SO_VM_SOCKETS_BUFFER_SIZE, vsock_buf_bytes,
+			"setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)"))
+		exit(EXIT_FAILURE);
 
-	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
-		       &vsock_buf_bytes, sizeof(vsock_buf_bytes)))
-		error("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
 }
 
 static int vsock_connect(unsigned int cid, unsigned int port)
@@ -183,10 +187,10 @@ static void run_receiver(int rcvlowat_bytes)
 
 	vsock_increase_buf_size(client_fd);
 
-	if (setsockopt(client_fd, SOL_SOCKET, SO_RCVLOWAT,
-		       &rcvlowat_bytes,
-		       sizeof(rcvlowat_bytes)))
-		error("setsockopt(SO_RCVLOWAT)");
+
+	if (!setsockopt_int_check(client_fd, SOL_SOCKET, SO_RCVLOWAT,
+			rcvlowat_bytes, "setsockopt(SO_RCVLOWAT)"))
+		exit(EXIT_FAILURE);
 
 	data = malloc(buf_size_bytes);
 
diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
index c7af23332e57..3764dca1118e 100644
--- a/tools/testing/vsock/vsock_test.c
+++ b/tools/testing/vsock/vsock_test.c
@@ -27,6 +27,7 @@
 #include "timeout.h"
 #include "control.h"
 #include "util.h"
+#include "util_socket.h"
 
 static void test_stream_connection_reset(const struct test_opts *opts)
 {
@@ -444,17 +445,14 @@ 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)");
+	if (!setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
+			sock_buf_size,
+			"setsockopt(SO_VM_SOCKETS_BUFFER_MAX_SIZE)"))
 		exit(EXIT_FAILURE);
-	}
 
-	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
-		       &sock_buf_size, sizeof(sock_buf_size))) {
-		perror("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
+	if (!setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
+		       sock_buf_size, "setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)"))
 		exit(EXIT_FAILURE);
-	}
 
 	/* Ready to receive data. */
 	control_writeln("SRVREADY");
@@ -586,10 +584,9 @@ 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)");
+	if (!setsockopt_timeval_check(fd, SOL_SOCKET, SO_RCVTIMEO, tv,
+			"setsockopt(SO_RCVTIMEO)"))
 		exit(EXIT_FAILURE);
-	}
 
 	read_enter_ns = current_nsec();
 
@@ -855,9 +852,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)");
+	if (!setsockopt_int_check(fd, SOL_SOCKET, SO_RCVLOWAT,
+		       lowat_val, "setsockopt(SO_RCVLOWAT)")) {
 		exit(EXIT_FAILURE);
 	}
 
@@ -1383,11 +1379,9 @@ 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)");
+	if (!setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
+		       sock_buf_size, "setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)"))
 		exit(EXIT_FAILURE);
-	}
 
 	if (low_rx_bytes_test) {
 		/* Set new SO_RCVLOWAT here. This enables sending credit
@@ -1396,9 +1390,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)");
+		if (!setsockopt_int_check(fd, SOL_SOCKET, SO_RCVLOWAT,
+			       recv_buf_size, "setsockopt(SO_RCVLOWAT)")) {
 			exit(EXIT_FAILURE);
 		}
 	}
@@ -1442,9 +1435,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)");
+		if (!setsockopt_int_check(fd, SOL_SOCKET, SO_RCVLOWAT,
+			       recv_buf_size, "setsockopt(SO_RCVLOWAT)")) {
 			exit(EXIT_FAILURE);
 		}
 	}
-- 
2.34.1

Re: [PATCH v5 2/3] vsock/test: fix parameter types in SO_VM_SOCKETS_* calls

[Stefano Garzarella]

On Thu, Nov 07, 2024 at 07:17:25PM -0600, Konstantin Shkolnyy wrote:
>Change parameters of SO_VM_SOCKETS_* to unsigned long long as documented
>in the vm_sockets.h, because the corresponding kernel code requires them
>to be at least 64-bit, no matter what architecture. Otherwise they are
>too small on 32-bit machines.
>
>Fixes: 5c338112e48a ("test/vsock: rework message bounds test")
>Fixes: 685a21c314a8 ("test/vsock: add big message test")
>Fixes: 542e893fbadc ("vsock/test: two tests to check credit update logic")
>Fixes: 8abbffd27ced ("test/vsock: vsock_perf utility")
>Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
>---
> tools/testing/vsock/vsock_perf.c |  4 ++--
> tools/testing/vsock/vsock_test.c | 22 +++++++++++++++++-----
> 2 files changed, 19 insertions(+), 7 deletions(-)

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

>
>diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c
>index 22633c2848cc..8e0a6c0770d3 100644
>--- a/tools/testing/vsock/vsock_perf.c
>+++ b/tools/testing/vsock/vsock_perf.c
>@@ -33,7 +33,7 @@
>
> static unsigned int port = DEFAULT_PORT;
> static unsigned long buf_size_bytes = DEFAULT_BUF_SIZE_BYTES;
>-static unsigned long vsock_buf_bytes = DEFAULT_VSOCK_BUF_BYTES;
>+static unsigned long long vsock_buf_bytes = DEFAULT_VSOCK_BUF_BYTES;
> static bool zerocopy;
>
> static void error(const char *s)
>@@ -162,7 +162,7 @@ static void run_receiver(int rcvlowat_bytes)
> 	printf("Run as receiver\n");
> 	printf("Listen port %u\n", port);
> 	printf("RX buffer %lu bytes\n", buf_size_bytes);
>-	printf("vsock buffer %lu bytes\n", vsock_buf_bytes);
>+	printf("vsock buffer %llu bytes\n", vsock_buf_bytes);
> 	printf("SO_RCVLOWAT %d bytes\n", rcvlowat_bytes);
>
> 	fd = socket(AF_VSOCK, SOCK_STREAM, 0);
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index 7fd25b814b4b..c7af23332e57 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -429,7 +429,7 @@ static void test_seqpacket_msg_bounds_client(const struct test_opts *opts)
>
> static void test_seqpacket_msg_bounds_server(const struct test_opts *opts)
> {
>-	unsigned long sock_buf_size;
>+	unsigned long long sock_buf_size;
> 	unsigned long remote_hash;
> 	unsigned long curr_hash;
> 	int fd;
>@@ -634,7 +634,8 @@ static void test_seqpacket_timeout_server(const struct test_opts *opts)
>
> static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
> {
>-	unsigned long sock_buf_size;
>+	unsigned long long sock_buf_size;
>+	size_t buf_size;
> 	socklen_t len;
> 	void *data;
> 	int fd;
>@@ -655,13 +656,20 @@ static void test_seqpacket_bigmsg_client(const struct test_opts *opts)
>
> 	sock_buf_size++;
>
>-	data = malloc(sock_buf_size);
>+	/* size_t can be < unsigned long long */
>+	buf_size = (size_t) sock_buf_size;
>+	if (buf_size != sock_buf_size) {
>+		fprintf(stderr, "Returned BUFFER_SIZE too large\n");
>+		exit(EXIT_FAILURE);
>+	}
>+
>+	data = malloc(buf_size);
> 	if (!data) {
> 		perror("malloc");
> 		exit(EXIT_FAILURE);
> 	}
>
>-	send_buf(fd, data, sock_buf_size, 0, -EMSGSIZE);
>+	send_buf(fd, data, buf_size, 0, -EMSGSIZE);
>
> 	control_writeln("CLISENT");
>
>@@ -1360,6 +1368,7 @@ static void test_stream_credit_update_test(const struct test_opts *opts,
> 	int recv_buf_size;
> 	struct pollfd fds;
> 	size_t buf_size;
>+	unsigned long long sock_buf_size;
> 	void *buf;
> 	int fd;
>
>@@ -1371,8 +1380,11 @@ static void test_stream_credit_update_test(const struct test_opts *opts,
>
> 	buf_size = RCVLOWAT_CREDIT_UPD_BUF_SIZE;
>
>+	/* size_t can be < unsigned long long */
>+	sock_buf_size = buf_size;
>+
> 	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>-		       &buf_size, sizeof(buf_size))) {
>+		       &sock_buf_size, sizeof(sock_buf_size))) {
> 		perror("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
> 		exit(EXIT_FAILURE);
> 	}
>-- 
>2.34.1
>

Re: [PATCH v5 3/3] vsock/test: verify socket options after setting them

[Stefano Garzarella]

On Thu, Nov 07, 2024 at 07:17:26PM -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.
>
>Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
>---
> tools/testing/vsock/Makefile              |   8 +-
> tools/testing/vsock/control.c             |   8 +-
> tools/testing/vsock/msg_zerocopy_common.c |   8 +-
> tools/testing/vsock/util_socket.c         | 149 ++++++++++++++++++++++
> tools/testing/vsock/util_socket.h         |  19 +++
> tools/testing/vsock/vsock_perf.c          |  24 ++--
> tools/testing/vsock/vsock_test.c          |  40 +++---
> 7 files changed, 208 insertions(+), 48 deletions(-)
> create mode 100644 tools/testing/vsock/util_socket.c
> create mode 100644 tools/testing/vsock/util_socket.h
>
>diff --git a/tools/testing/vsock/Makefile b/tools/testing/vsock/Makefile
>index 6e0b4e95e230..1ec0b3a67aa4 100644
>--- a/tools/testing/vsock/Makefile
>+++ b/tools/testing/vsock/Makefile
>@@ -1,12 +1,12 @@
> # SPDX-License-Identifier: GPL-2.0-only
> all: test vsock_perf
> test: vsock_test vsock_diag_test vsock_uring_test
>-vsock_test: vsock_test.o vsock_test_zerocopy.o timeout.o control.o util.o msg_zerocopy_common.o
>-vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o
>-vsock_perf: vsock_perf.o msg_zerocopy_common.o
>+vsock_test: vsock_test.o vsock_test_zerocopy.o timeout.o control.o util.o msg_zerocopy_common.o util_socket.o
>+vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o util_socket.o
>+vsock_perf: vsock_perf.o msg_zerocopy_common.o util_socket.o

I would add the new functions to check setsockopt in util.c

vsock_perf is more of a tool to measure performance than a test, so
we can avoid calling these checks there, tests should cover all
cases regardless of vsock_perf.

>
> vsock_uring_test: LDLIBS = -luring
>-vsock_uring_test: control.o util.o vsock_uring_test.o timeout.o msg_zerocopy_common.o
>+vsock_uring_test: control.o util.o vsock_uring_test.o timeout.o msg_zerocopy_common.o util_socket.o
>
> CFLAGS += -g -O2 -Werror -Wall -I. -I../../include 
> -I../../../usr/include -Wno-pointer-sign -fno-strict-overflow 
> -fno-strict-aliasing -fno-common -MMD -U_FORTIFY_SOURCE -D_GNU_SOURCE
> .PHONY: all test clean
>diff --git a/tools/testing/vsock/control.c b/tools/testing/vsock/control.c
>index d2deb4b15b94..f1fd809ac9d5 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_socket.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,9 @@ void control_init(const char *control_host,
> 			break;
> 		}
>
>-		if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR,
>-			       &val, sizeof(val)) < 0) {
>-			perror("setsockopt");
>+		if (!setsockopt_int_check(fd, SOL_SOCKET, SO_REUSEADDR, 1,
>+				"setsockopt SO_REUSEADDR"))
> 			exit(EXIT_FAILURE);
>-		}
>
> 		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..4edb1b6974c0 100644
>--- a/tools/testing/vsock/msg_zerocopy_common.c
>+++ b/tools/testing/vsock/msg_zerocopy_common.c
>@@ -13,15 +13,13 @@
> #include <linux/errqueue.h>
>
> #include "msg_zerocopy_common.h"
>+#include "util_socket.h"
>
> void enable_so_zerocopy(int fd)
> {
>-	int val = 1;
>-
>-	if (setsockopt(fd, SOL_SOCKET, SO_ZEROCOPY, &val, sizeof(val))) {
>-		perror("setsockopt");
>+	if (!setsockopt_int_check(fd, SOL_SOCKET, SO_ZEROCOPY, 1,
>+			"setsockopt SO_ZEROCOPY"))
> 		exit(EXIT_FAILURE);
>-	}
> }
>
> void vsock_recv_completion(int fd, const bool *zerocopied)
>diff --git a/tools/testing/vsock/util_socket.c b/tools/testing/vsock/util_socket.c
>new file mode 100644
>index 000000000000..e791da160624
>--- /dev/null
>+++ b/tools/testing/vsock/util_socket.c
>@@ -0,0 +1,149 @@
>+// SPDX-License-Identifier: GPL-2.0-only
>+/*
>+ * Socket utility functions.
>+ *
>+ * Copyright IBM Corp. 2024
>+ */
>+#include <errno.h>
>+#include <inttypes.h>
>+#include <stdio.h>
>+#include <string.h>
>+#include <sys/socket.h>
>+#include "util_socket.h"
>+
>+/* Set "unsigned long long" socket option and check that it's indeed set */
>+bool 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 true;
>+fail:
>+	fprintf(stderr, "%s  val %llu\n", errmsg, val);
>+	return false;
>+}
>+
>+/* Set "int" socket option and check that it's indeed set */
>+bool 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 true;
>+fail:
>+	fprintf(stderr, "%s val %d\n", errmsg, val);
>+	return false;
>+}
>+
>+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 */
>+bool 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 true;
>+fail:
>+	fprintf(stderr, "%s val %ld:%ld\n", errmsg, val.tv_sec, val.tv_usec);
>+	return false;
>+}
>diff --git a/tools/testing/vsock/util_socket.h b/tools/testing/vsock/util_socket.h
>new file mode 100644
>index 000000000000..38cf3decb15c
>--- /dev/null
>+++ b/tools/testing/vsock/util_socket.h
>@@ -0,0 +1,19 @@
>+/* SPDX-License-Identifier: GPL-2.0-only */
>+/*
>+ * Socket utility functions.
>+ *
>+ * Copyright IBM Corp. 2024
>+ */
>+#ifndef UTIL_SOCKET_H
>+#define UTIL_SOCKET_H
>+
>+#include <stdbool.h>
>+
>+bool setsockopt_ull_check(int fd, int level, int optname,
>+		unsigned long long val, char const *errmsg);
>+bool setsockopt_int_check(int fd, int level, int optname, int val,
>+		char const *errmsg);
>+bool setsockopt_timeval_check(int fd, int level, int optname,
>+		struct timeval val, char const *errmsg);

We call of them in the same way in the tests:

if (!setsockopt...check(...))
     exit(EXIT_FAILURE);

So, what about making them void and calling exit in the functions?

We already do this in other functions.

Thanks for this work!
Stefano

>+
>+#endif /* UTIL_SOCKET_H */
>diff --git a/tools/testing/vsock/vsock_perf.c b/tools/testing/vsock/vsock_perf.c
>index 8e0a6c0770d3..b117e043b87b 100644
>--- a/tools/testing/vsock/vsock_perf.c
>+++ b/tools/testing/vsock/vsock_perf.c
>@@ -21,6 +21,7 @@
> #include <sys/mman.h>
>
> #include "msg_zerocopy_common.h"
>+#include "util_socket.h"
>
> #define DEFAULT_BUF_SIZE_BYTES	(128 * 1024)
> #define DEFAULT_TO_SEND_BYTES	(64 * 1024)
>@@ -88,13 +89,16 @@ static unsigned long memparse(const char *ptr)
>
> static void vsock_increase_buf_size(int fd)
> {
>-	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>-		       &vsock_buf_bytes, sizeof(vsock_buf_bytes)))
>-		error("setsockopt(SO_VM_SOCKETS_BUFFER_MAX_SIZE)");
>+	if (!setsockopt_ull_check(fd, AF_VSOCK,
>+			SO_VM_SOCKETS_BUFFER_MAX_SIZE, vsock_buf_bytes,
>+			"setsockopt(SO_VM_SOCKETS_BUFFER_MAX_SIZE)"))
>+		exit(EXIT_FAILURE);
>+
>+	if (!setsockopt_ull_check(fd, AF_VSOCK,
>+			SO_VM_SOCKETS_BUFFER_SIZE, vsock_buf_bytes,
>+			"setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)"))
>+		exit(EXIT_FAILURE);
>
>-	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>-		       &vsock_buf_bytes, sizeof(vsock_buf_bytes)))
>-		error("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
> }
>
> static int vsock_connect(unsigned int cid, unsigned int port)
>@@ -183,10 +187,10 @@ static void run_receiver(int rcvlowat_bytes)
>
> 	vsock_increase_buf_size(client_fd);
>
>-	if (setsockopt(client_fd, SOL_SOCKET, SO_RCVLOWAT,
>-		       &rcvlowat_bytes,
>-		       sizeof(rcvlowat_bytes)))
>-		error("setsockopt(SO_RCVLOWAT)");
>+
>+	if (!setsockopt_int_check(client_fd, SOL_SOCKET, SO_RCVLOWAT,
>+			rcvlowat_bytes, "setsockopt(SO_RCVLOWAT)"))
>+		exit(EXIT_FAILURE);
>
> 	data = malloc(buf_size_bytes);
>
>diff --git a/tools/testing/vsock/vsock_test.c b/tools/testing/vsock/vsock_test.c
>index c7af23332e57..3764dca1118e 100644
>--- a/tools/testing/vsock/vsock_test.c
>+++ b/tools/testing/vsock/vsock_test.c
>@@ -27,6 +27,7 @@
> #include "timeout.h"
> #include "control.h"
> #include "util.h"
>+#include "util_socket.h"
>
> static void test_stream_connection_reset(const struct test_opts *opts)
> {
>@@ -444,17 +445,14 @@ 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)");
>+	if (!setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_MAX_SIZE,
>+			sock_buf_size,
>+			"setsockopt(SO_VM_SOCKETS_BUFFER_MAX_SIZE)"))
> 		exit(EXIT_FAILURE);
>-	}
>
>-	if (setsockopt(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>-		       &sock_buf_size, sizeof(sock_buf_size))) {
>-		perror("setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)");
>+	if (!setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>+		       sock_buf_size, "setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)"))
> 		exit(EXIT_FAILURE);
>-	}
>
> 	/* Ready to receive data. */
> 	control_writeln("SRVREADY");
>@@ -586,10 +584,9 @@ 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)");
>+	if (!setsockopt_timeval_check(fd, SOL_SOCKET, SO_RCVTIMEO, tv,
>+			"setsockopt(SO_RCVTIMEO)"))
> 		exit(EXIT_FAILURE);
>-	}
>
> 	read_enter_ns = current_nsec();
>
>@@ -855,9 +852,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)");
>+	if (!setsockopt_int_check(fd, SOL_SOCKET, SO_RCVLOWAT,
>+		       lowat_val, "setsockopt(SO_RCVLOWAT)")) {
> 		exit(EXIT_FAILURE);
> 	}
>
>@@ -1383,11 +1379,9 @@ 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)");
>+	if (!setsockopt_ull_check(fd, AF_VSOCK, SO_VM_SOCKETS_BUFFER_SIZE,
>+		       sock_buf_size, "setsockopt(SO_VM_SOCKETS_BUFFER_SIZE)"))
> 		exit(EXIT_FAILURE);
>-	}
>
> 	if (low_rx_bytes_test) {
> 		/* Set new SO_RCVLOWAT here. This enables sending credit
>@@ -1396,9 +1390,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)");
>+		if (!setsockopt_int_check(fd, SOL_SOCKET, SO_RCVLOWAT,
>+			       recv_buf_size, "setsockopt(SO_RCVLOWAT)")) {
> 			exit(EXIT_FAILURE);
> 		}
> 	}
>@@ -1442,9 +1435,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)");
>+		if (!setsockopt_int_check(fd, SOL_SOCKET, SO_RCVLOWAT,
>+			       recv_buf_size, "setsockopt(SO_RCVLOWAT)")) {
> 			exit(EXIT_FAILURE);
> 		}
> 	}
>-- 
>2.34.1
>

Re: [PATCH v5 3/3] vsock/test: verify socket options after setting them

[Konstantin Shkolnyy]

On 11/12/2024 02:58, Stefano Garzarella wrote:
> On Thu, Nov 07, 2024 at 07:17:26PM -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.
>>
>> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
>> ---
>> tools/testing/vsock/Makefile              |   8 +-
>> tools/testing/vsock/control.c             |   8 +-
>> tools/testing/vsock/msg_zerocopy_common.c |   8 +-
>> tools/testing/vsock/util_socket.c         | 149 ++++++++++++++++++++++
>> tools/testing/vsock/util_socket.h         |  19 +++
>> tools/testing/vsock/vsock_perf.c          |  24 ++--
>> tools/testing/vsock/vsock_test.c          |  40 +++---
>> 7 files changed, 208 insertions(+), 48 deletions(-)
>> create mode 100644 tools/testing/vsock/util_socket.c
>> create mode 100644 tools/testing/vsock/util_socket.h
>>
>> diff --git a/tools/testing/vsock/Makefile b/tools/testing/vsock/Makefile
>> index 6e0b4e95e230..1ec0b3a67aa4 100644
>> --- a/tools/testing/vsock/Makefile
>> +++ b/tools/testing/vsock/Makefile
>> @@ -1,12 +1,12 @@
>> # SPDX-License-Identifier: GPL-2.0-only
>> all: test vsock_perf
>> test: vsock_test vsock_diag_test vsock_uring_test
>> -vsock_test: vsock_test.o vsock_test_zerocopy.o timeout.o control.o 
>> util.o msg_zerocopy_common.o
>> -vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o
>> -vsock_perf: vsock_perf.o msg_zerocopy_common.o
>> +vsock_test: vsock_test.o vsock_test_zerocopy.o timeout.o control.o 
>> util.o msg_zerocopy_common.o util_socket.o
>> +vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o 
>> util_socket.o
>> +vsock_perf: vsock_perf.o msg_zerocopy_common.o util_socket.o
> 
> I would add the new functions to check setsockopt in util.c
> 
> vsock_perf is more of a tool to measure performance than a test, so
> we can avoid calling these checks there, tests should cover all
> cases regardless of vsock_perf.

The problem is that vsock_perf calls enable_so_zerocopy() which has to
call the new setsockopt_int_check() because it's also called by 
vsock_test. Do you prefer to give vsock_perf its own version of
enable_so_zerocopy() which doesn't call setsockopt_int_check()?

Re: [PATCH v5 3/3] vsock/test: verify socket options after setting them

[Stefano Garzarella]

On Tue, Nov 12, 2024 at 09:18:48AM -0600, Konstantin Shkolnyy wrote:
>On 11/12/2024 02:58, Stefano Garzarella wrote:
>>On Thu, Nov 07, 2024 at 07:17:26PM -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.
>>>
>>>Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
>>>---
>>>tools/testing/vsock/Makefile              |   8 +-
>>>tools/testing/vsock/control.c             |   8 +-
>>>tools/testing/vsock/msg_zerocopy_common.c |   8 +-
>>>tools/testing/vsock/util_socket.c         | 149 ++++++++++++++++++++++
>>>tools/testing/vsock/util_socket.h         |  19 +++
>>>tools/testing/vsock/vsock_perf.c          |  24 ++--
>>>tools/testing/vsock/vsock_test.c          |  40 +++---
>>>7 files changed, 208 insertions(+), 48 deletions(-)
>>>create mode 100644 tools/testing/vsock/util_socket.c
>>>create mode 100644 tools/testing/vsock/util_socket.h
>>>
>>>diff --git a/tools/testing/vsock/Makefile b/tools/testing/vsock/Makefile
>>>index 6e0b4e95e230..1ec0b3a67aa4 100644
>>>--- a/tools/testing/vsock/Makefile
>>>+++ b/tools/testing/vsock/Makefile
>>>@@ -1,12 +1,12 @@
>>># SPDX-License-Identifier: GPL-2.0-only
>>>all: test vsock_perf
>>>test: vsock_test vsock_diag_test vsock_uring_test
>>>-vsock_test: vsock_test.o vsock_test_zerocopy.o timeout.o 
>>>control.o util.o msg_zerocopy_common.o
>>>-vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o
>>>-vsock_perf: vsock_perf.o msg_zerocopy_common.o
>>>+vsock_test: vsock_test.o vsock_test_zerocopy.o timeout.o 
>>>control.o util.o msg_zerocopy_common.o util_socket.o
>>>+vsock_diag_test: vsock_diag_test.o timeout.o control.o util.o 
>>>util_socket.o
>>>+vsock_perf: vsock_perf.o msg_zerocopy_common.o util_socket.o
>>
>>I would add the new functions to check setsockopt in util.c
>>
>>vsock_perf is more of a tool to measure performance than a test, so
>>we can avoid calling these checks there, tests should cover all
>>cases regardless of vsock_perf.
>
>The problem is that vsock_perf calls enable_so_zerocopy() which has to
>call the new setsockopt_int_check() because it's also called by 
>vsock_test. Do you prefer to give vsock_perf its own version of
>enable_so_zerocopy() which doesn't call setsockopt_int_check()?
>

Yeah, maybe we can move the old enable_so_zerocopy() in vsock_perf.c
and implement another enable_so_zerocopy() in util.c for the tests.

Thanks,
Stefano