On Mon, Sep 2, 2024 at 11:02 PM Willem de Bruijn willemdebruijn.kernel@gmail.com wrote:
Jason Xing wrote:
From: Jason Xing kernelxing@tencent.com
Add the SO_PEEK_OFF selftest for UDP. In this patch, I mainly do three things:
- rename tcp_so_peek_off.c
- adjust for UDP protocol
- add selftests into it
Suggested-by: Jon Maloy jmaloy@redhat.com Signed-off-by: Jason Xing kernelxing@tencent.com
A few minor comments. Nothing important.
Subject to Stan's point about .gitignore:
Reviewed-by: Willem de Bruijn willemb@google.com
Thanks for your review!
-int tcp_peek_offset_probe(sa_family_t af) +int sk_peek_offset_probe(sa_family_t af, int proto) {
int type = (proto == IPPROTO_TCP ? SOCK_STREAM : SOCK_DGRAM); int optv = 0; int ret = 0; int s;
s = socket(af, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);
s = socket(af, type, proto);Removing the SOCK_CLOEXEC because not relevant to this single thread process, I suppose?
Yep. We don't need this one.
Not important, but no need for proto, can just be 0.
You're right. I wonder if it is better if we explicitly pass the proto here? I would like not to touch it here.
if (s < 0) { ksft_perror("Temporary TCP socket creation failed"); } else { if (!setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &optv, sizeof(int))) ret = 1; else
printf("%s does not support SO_PEEK_OFF\n", afstr(af));
printf("%s does not support SO_PEEK_OFF\n", afstr(af, proto)); close(s); } return ret;}
-static void tcp_peek_offset_set(int s, int offset) +static void sk_peek_offset_set(int s, int offset) { if (setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &offset, sizeof(offset))) ksft_perror("Failed to set SO_PEEK_OFF value\n"); }
-static int tcp_peek_offset_get(int s) +static int sk_peek_offset_get(int s) { int offset; socklen_t len = sizeof(offset); @@ -50,8 +54,9 @@ static int tcp_peek_offset_get(int s) return offset; }
-static int tcp_peek_offset_test(sa_family_t af) +static int sk_peek_offset_test(sa_family_t af, int proto) {
int type = (proto == IPPROTO_TCP ? SOCK_STREAM : SOCK_DGRAM); union { struct sockaddr sa; struct sockaddr_in a4;@@ -62,13 +67,13 @@ static int tcp_peek_offset_test(sa_family_t af) int recv_sock = 0; int offset = 0; ssize_t len;
char buf;
char buf[2]; memset(&a, 0, sizeof(a)); a.sa.sa_family = af;
s[0] = socket(af, SOCK_STREAM, IPPROTO_TCP);s[1] = socket(af, SOCK_STREAM | SOCK_NONBLOCK, IPPROTO_TCP);
s[0] = recv_sock = socket(af, type, proto);s[1] = socket(af, type, proto);Same
I think we don't need this one, either. As we can see, there are already some existing test files without the SOCK_NONBLOCK flag.
if (s[0] < 0 || s[1] < 0) { ksft_perror("Temporary socket creation failed\n");@@ -82,76 +87,78 @@ static int tcp_peek_offset_test(sa_family_t af) ksft_perror("Temporary socket getsockname() failed\n"); goto out; }
if (listen(s[0], 0) < 0) {
if (proto == IPPROTO_TCP && listen(s[0], 0) < 0) { ksft_perror("Temporary socket listen() failed\n"); goto out; }
if (connect(s[1], &a.sa, sizeof(a)) >= 0 || errno != EINPROGRESS) {
if (connect(s[1], &a.sa, sizeof(a))) { ksft_perror("Temporary socket connect() failed\n"); goto out; }Changed due to the removal of SOCK_NONBLOCK above. Definitely simplifies the test.
Yep.
Just note that error test is == -1 or < 0, also for consistency with the rest of the file.
I will add "< 0" here as you said.
Thanks, Jason