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

[Stefano Garzarella <sgarzare@xxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>

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