On Mon, Jul 22, 2024 at 03:07 PM +02, Michal Luczaj wrote:
> On 7/19/24 13:09, Jakub Sitnicki wrote:
>> On Wed, Jul 17, 2024 at 10:15 PM +02, Michal Luczaj wrote:
>>> On 7/13/24 11:45, Jakub Sitnicki wrote:
>>>> On Thu, Jul 11, 2024 at 10:33 PM +02, Michal Luczaj wrote:
>>>>> 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.
>
>> 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@xxxxxxxxxx/
>
> 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;
> }
