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.1On 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
>
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()?
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
© 2016 - 2024 Red Hat, Inc.