[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <66d5d38c1d4b_61388294f0@willemb.c.googlers.com.notmuch>
Date: Mon, 02 Sep 2024 11:02:36 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>,
davem@...emloft.net,
edumazet@...gle.com,
kuba@...nel.org,
pabeni@...hat.com,
shuah@...nel.org,
jmaloy@...hat.com
Cc: linux-kselftest@...r.kernel.org,
netdev@...r.kernel.org,
Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next] selftests: add selftest for UDP SO_PEEK_OFF
support
Jason Xing wrote:
> From: Jason Xing <kernelxing@...cent.com>
>
> Add the SO_PEEK_OFF selftest for UDP. In this patch, I mainly do
> three things:
> 1. rename tcp_so_peek_off.c
> 2. adjust for UDP protocol
> 3. add selftests into it
>
> Suggested-by: Jon Maloy <jmaloy@...hat.com>
> Signed-off-by: Jason Xing <kernelxing@...cent.com>
A few minor comments. Nothing important.
Subject to Stan's point about .gitignore:
Reviewed-by: Willem de Bruijn <willemb@...gle.com>
> -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?
Not important, but no need for proto, can just be 0.
> 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
>
> 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.
Just note that error test is == -1 or < 0, also for consistency with
the rest of the file.
> - recv_sock = accept(s[0], NULL, NULL);
> - if (recv_sock <= 0) {
> - ksft_perror("Temporary socket accept() failed\n");
> - goto out;
> + if (proto == IPPROTO_TCP) {
> + recv_sock = accept(s[0], NULL, NULL);
> + if (recv_sock <= 0) {
> + ksft_perror("Temporary socket accept() failed\n");
> + goto out;
> + }
Powered by blists - more mailing lists