On 7/22/24 21:26, Jakub Sitnicki wrote: > On Mon, Jul 22, 2024 at 03:07 PM +02, Michal Luczaj wrote: >>> 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@xxxxxxx >> [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. 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. >> 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.
