On Mon, Jul 22, 2024 at 03:07 PM +02, Michal Luczaj wrote:
On 7/19/24 13:09, Jakub Sitnicki wrote:
quoted
On Wed, Jul 17, 2024 at 10:15 PM +02, Michal Luczaj wrote:
quoted
On 7/13/24 11:45, Jakub Sitnicki wrote:
quoted
On Thu, Jul 11, 2024 at 10:33 PM +02, Michal Luczaj wrote:
quoted
And looking at that commit[1], inet_unix_redir_to_connected() has its
@type ignored, too. Same treatment?
That one will not be a trivial fix like this case. inet_socketpair()
won't work for TCP as is. It will fail trying to connect() a listening
socket (p0). I recall now that we are in this state due to some
abandoned work that began in 75e0e27db6cf ("selftest/bpf: Change udp to
inet in some function names").
[...]
Is this what you've meant? With this patch inet_socketpair() and
vsock_socketpair_connectible can be reduced to a single call to
create_pair(). And pairs creation in inet_unix_redir_to_connected()
and unix_inet_redir_to_connected() accepts both sotypes.
Yes, exactly. This looks great.
Happy to hear that. I'll prepare a series, include the little fixes and
send it out for a proper review.
One more thing: I've noticed changes in sockmap_helpers.h don't trigger
test_progs rebuild (seems to be the case for all .h in prog_tests/). No
idea if this is the right approach, but adding
"$(TRUNNER_TESTS_DIR)/sockmap_helpers.h" to TRUNNER_EXTRA_SOURCES in
selftests/bpf/Makefile does the trick.
CC'ed BPF selftests reviewers in case they'd like to chip in.
quoted
Classic cleanup with goto to close sockets is all right, but if you're
feeling brave and aim for something less branchy, I've noticed we have
finally started using __attribute__((cleanup)):
https://elixir.bootlin.com/linux/v6.10/source/tools/testing/selftests/bpf/progs/iters.c#L115
I've tried. Is such "ownership passing" (to inhibit the cleanup) via
construct like take_fd()[1] welcomed?
I'm fine with having such a helper to complement the cleanup attribute.
Alternatively, we can always open code it like it used to be in systemd
at first [1], if other reviewers don't warm up to it :-)
[1] https://github.com/systemd/systemd/blob/main/coccinelle/take-fd.cocci
[1] https://lore.kernel.org/all/20240627-work-pidfs-v1-1-7e9ab6cc3bb1@kernel.org/
static inline void close_fd(int *fd)
{
if (*fd >= 0)
xclose(*fd);
}
#define __closefd __attribute__((cleanup(close_fd)))
static inline int create_pair(int family, int sotype, int *c, int *p)
{
struct sockaddr_storage addr;
socklen_t len = sizeof(addr);
int err;
int s __closefd = socket_loopback(family, sotype);
if (s < 0)
return s;
err = xgetsockname(s, sockaddr(&addr), &len);
if (err)
return err;
int s0 __closefd = xsocket(family, sotype, 0);
I'd stick to no declarations in the body. Init to -1 or -EBADF.
if (s0 < 0)
return s0;
err = connect(s0, sockaddr(&addr), len);
if (err) {
if (errno != EINPROGRESS) {
FAIL_ERRNO("connect");
return err;
}
err = poll_connect(s0, IO_TIMEOUT_SEC);
if (err) {
FAIL_ERRNO("poll_connect");
return err;
}
}
switch (sotype & SOCK_TYPE_MASK) {
case SOCK_DGRAM:
err = xgetsockname(s0, sockaddr(&addr), &len);
if (err)
return err;
err = xconnect(s, sockaddr(&addr), len);
if (err)
return err;
*p = take_fd(s);
break;
case SOCK_STREAM:
case SOCK_SEQPACKET:
*p = xaccept_nonblock(s, NULL, NULL);
I wouldn't touch output arguments until we have succedeed. Another
local var will be handy.
if (*p < 0)
return *p;
break;
default:
FAIL("Unsupported socket type %#x", sotype);
return -EOPNOTSUPP;
}
*c = take_fd(s0);
return err;
}
On Mon, 2024-07-22 at 21:26 +0200, Jakub Sitnicki wrote:
On Mon, Jul 22, 2024 at 03:07 PM +02, Michal Luczaj wrote:
[...]
quoted
One more thing: I've noticed changes in sockmap_helpers.h don't trigger
test_progs rebuild (seems to be the case for all .h in prog_tests/). No
idea if this is the right approach, but adding
"$(TRUNNER_TESTS_DIR)/sockmap_helpers.h" to TRUNNER_EXTRA_SOURCES in
selftests/bpf/Makefile does the trick.
CC'ed BPF selftests reviewers in case they'd like to chip in.
Are you sure this is reproducible?
I tried the following:
$ make clean
$ make -j test_progs
$ touch prog_tests/sockmap_helpers.h
$ make -j test_progs
And I see the following files being remade:
TEST-OBJ [test_progs] sockmap_basic.test.o
TEST-OBJ [test_progs] sockmap_listen.test.o
TEST-OBJ [test_progs] verifier.test.o
BINARY test_progs
(Although, there are a few other files,
that probably should not be remade, need to look into it).
Also, here is some debug output:
$ make -j24 --print-data-base | grep "sockmap_basic.test.o:" | tr ' ' '\n' | grep '\(:\|sockmap_helpers.h\)'
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/cpuv4/sockmap_basic.test.o:
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/sockmap_basic.test.o:
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/no_alu32/sockmap_basic.test.o:
/home/eddy/work/bpf-next/tools/testing/selftests/bpf/prog_tests/sockmap_helpers.h
[...]
On Mon, 2024-07-22 at 15:07 -0700, Eduard Zingerman wrote:
[...]
Digging a little bit further, I think the behaviour mentioned was fixed
recently by the following commit:
a3cc56cd2c20 ("selftests/bpf: Use auto-dependencies for test objects")
From 3 days ago.
As the dependency is set from sockmap_basic.test.d,
generated while sockmap_basic.test.o is compiled.
On 7/23/24 00:21, Eduard Zingerman wrote:
On Mon, 2024-07-22 at 15:07 -0700, Eduard Zingerman wrote:
[...]
Digging a little bit further, I think the behaviour mentioned was fixed
recently by the following commit:
a3cc56cd2c20 ("selftests/bpf: Use auto-dependencies for test objects")
From 3 days ago.
As the dependency is set from sockmap_basic.test.d,
generated while sockmap_basic.test.o is compiled.
Ah, yes, you're right: bpf-next works for me. Thank you very much for
solving this. And I apologise for the noise.
Michal
On 7/22/24 21:26, Jakub Sitnicki wrote:
On Mon, Jul 22, 2024 at 03:07 PM +02, Michal Luczaj wrote:
quoted
quoted
Classic cleanup with goto to close sockets is all right, but if you're
feeling brave and aim for something less branchy, I've noticed we have
finally started using __attribute__((cleanup)):
https://elixir.bootlin.com/linux/v6.10/source/tools/testing/selftests/bpf/progs/iters.c#L115
I've tried. Is such "ownership passing" (to inhibit the cleanup) via
construct like take_fd()[1] welcomed?
I'm fine with having such a helper to complement the cleanup attribute.
Alternatively, we can always open code it like it used to be in systemd
at first [1], if other reviewers don't warm up to it :-)
[1] https://github.com/systemd/systemd/blob/main/coccinelle/take-fd.cocci
OK, so I've kept create_pair()'s __cleanupfication as the last part of the
series:
https://lore.kernel.org/netdev/20240724-sockmap-selftest-fixes-v1-0-46165d224712@rbox.co
quoted
[1] https://lore.kernel.org/all/20240627-work-pidfs-v1-1-7e9ab6cc3bb1@kernel.org/
static inline void close_fd(int *fd)
{
if (*fd >= 0)
xclose(*fd);
}
#define __closefd __attribute__((cleanup(close_fd)))
static inline int create_pair(int family, int sotype, int *c, int *p)
{
struct sockaddr_storage addr;
socklen_t len = sizeof(addr);
int err;
int s __closefd = socket_loopback(family, sotype);
if (s < 0)
return s;
err = xgetsockname(s, sockaddr(&addr), &len);
if (err)
return err;
int s0 __closefd = xsocket(family, sotype, 0);
I'd stick to no declarations in the body. Init to -1 or -EBADF.
All right, it just felt wrong to (demand to) initialize variables with some
magic values. __attribute__((setup(set_negative))) would solve that :) I've
toyed with `DEFINE_CLASS(fd, int, if (_T >= 0) xclose(_T), -EBADF, void)`
but it felt wrong, too.
quoted
case SOCK_STREAM:
case SOCK_SEQPACKET:
*p = xaccept_nonblock(s, NULL, NULL);
I wouldn't touch output arguments until we have succedeed. Another
local var will be handy.
OK, sure. Thanks.