| Message ID | bf2359639287db9adef2c4ddc1a5e16e466a594a.1589405669.git.rdna@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: sk lookup, cgroup id helpers in cgroup skb | expand |
On 5/13/20 2:38 PM, Andrey Ignatov wrote: > Add two new network helpers. > > connect_fd_to_fd connects an already created client socket fd to address > of server fd. Sometimes it's useful to separate client socket creation > and connecting this socket to a server, e.g. if client socket has to be > created in a cgroup different from that of server cgroup. > > Additionally connect_to_fd is now implemented using connect_fd_to_fd, > both helpers don't treat EINPROGRESS as an error and let caller decide > how to proceed with it. > > connect_wait is a helper to work with non-blocking client sockets so > that if connect_to_fd or connect_fd_to_fd returned -1 with errno == > EINPROGRESS, caller can wait for connect to finish or for connection > timeout. The helper returns -1 on error, 0 on timeout (1sec, > hard-coded), and positive number on success. > > Signed-off-by: Andrey Ignatov <rdna@fb.com> Ack with a minor nit below. Acked-by: Yonghong Song <yhs@fb.com> > --- > tools/testing/selftests/bpf/network_helpers.c | 66 +++++++++++++++---- > tools/testing/selftests/bpf/network_helpers.h | 2 + > 2 files changed, 56 insertions(+), 12 deletions(-) > > diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c > index 0ff64b70b746..542d71ed7f5d 100644 > --- a/tools/testing/selftests/bpf/network_helpers.c > +++ b/tools/testing/selftests/bpf/network_helpers.c > @@ -4,10 +4,14 @@ > #include <stdio.h> > #include <string.h> > #include <unistd.h> > + > +#include <sys/epoll.h> > + > #include <linux/err.h> > #include <linux/in.h> > #include <linux/in6.h> > > +#include "bpf_util.h" > #include "network_helpers.h" > > #define clean_errno() (errno == 0 ? "None" : strerror(errno)) > @@ -77,8 +81,6 @@ static const size_t timeo_optlen = sizeof(timeo_sec); > > int connect_to_fd(int family, int type, int server_fd) > { > - struct sockaddr_storage addr; > - socklen_t len = sizeof(addr); > int fd; > > fd = socket(family, type, 0); > @@ -87,24 +89,64 @@ int connect_to_fd(int family, int type, int server_fd) > return -1; > } > > - if (setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeo_sec, timeo_optlen)) { > + if (connect_fd_to_fd(fd, server_fd) < 0 && errno != EINPROGRESS) { > + close(fd); Remote possibility. close(fd) may change error code? In my opinion, maybe convert the original syscall failure errno to return value and carrying on might be a simpler choice? > + return -1; > + } > + > + return fd; > +} > + > +int connect_fd_to_fd(int client_fd, int server_fd) > +{ > + struct sockaddr_storage addr; > + socklen_t len = sizeof(addr); > + > + if (setsockopt(client_fd, SOL_SOCKET, SO_RCVTIMEO, &timeo_sec, > + timeo_optlen)) { > log_err("Failed to set SO_RCVTIMEO"); > - goto out; > + return -1; > } > > if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) { > log_err("Failed to get server addr"); > - goto out; > + return -1; > } > > - if (connect(fd, (const struct sockaddr *)&addr, len) < 0) { > - log_err("Fail to connect to server with family %d", family); > - goto out; > + if (connect(client_fd, (const struct sockaddr *)&addr, len) < 0) { > + if (errno != EINPROGRESS) > + log_err("Failed to connect to server"); Not saying it is possible, but any remote possibility log_err() may change error code to EINPROGRESS? > + return -1; > } > > - return fd; > + return 0; > +} [...]
Yonghong Song <yhs@fb.com> [Thu, 2020-05-14 08:56 -0700]: > On 5/13/20 2:38 PM, Andrey Ignatov wrote: > > @@ -77,8 +81,6 @@ static const size_t timeo_optlen = sizeof(timeo_sec); > > int connect_to_fd(int family, int type, int server_fd) > > { > > - struct sockaddr_storage addr; > > - socklen_t len = sizeof(addr); > > int fd; > > fd = socket(family, type, 0); > > @@ -87,24 +89,64 @@ int connect_to_fd(int family, int type, int server_fd) > > return -1; > > } > > - if (setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeo_sec, timeo_optlen)) { > > + if (connect_fd_to_fd(fd, server_fd) < 0 && errno != EINPROGRESS) { > > + close(fd); > > Remote possibility. close(fd) may change error code? It can in some cases that are rather theoritical in this case (e.g. buggy multi-threaded program closes fd from another thread right before this close()). But I can save/restore it before/after close just in case. > In my opinion, maybe convert the original syscall failure errno to return > value and carrying on might be a simpler choice? I wanted to preserve semantics of connect(2) here: return -1 on error, or fd >= 0 on success. I guess if I save/restore errno it should be fine. > > + return -1; > > + } > > + > > + return fd; > > +} > > + > > +int connect_fd_to_fd(int client_fd, int server_fd) > > +{ > > + struct sockaddr_storage addr; > > + socklen_t len = sizeof(addr); > > + > > + if (setsockopt(client_fd, SOL_SOCKET, SO_RCVTIMEO, &timeo_sec, > > + timeo_optlen)) { > > log_err("Failed to set SO_RCVTIMEO"); > > - goto out; > > + return -1; > > } > > if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) { > > log_err("Failed to get server addr"); > > - goto out; > > + return -1; > > } > > - if (connect(fd, (const struct sockaddr *)&addr, len) < 0) { > > - log_err("Fail to connect to server with family %d", family); > > - goto out; > > + if (connect(client_fd, (const struct sockaddr *)&addr, len) < 0) { > > + if (errno != EINPROGRESS) > > + log_err("Failed to connect to server"); > > Not saying it is possible, but any remote possibility log_err() > may change error code to EINPROGRESS? To my best knowledge, neither fprintf(3) nor strerror(3) use EINPROGRESS, but since in this case having a reliable way to communicate EINPROGRESS from connect is rather required, I'll save/restore errno for caling log_err.
diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c index 0ff64b70b746..542d71ed7f5d 100644 --- a/tools/testing/selftests/bpf/network_helpers.c +++ b/tools/testing/selftests/bpf/network_helpers.c @@ -4,10 +4,14 @@ #include <stdio.h> #include <string.h> #include <unistd.h> + +#include <sys/epoll.h> + #include <linux/err.h> #include <linux/in.h> #include <linux/in6.h> +#include "bpf_util.h" #include "network_helpers.h" #define clean_errno() (errno == 0 ? "None" : strerror(errno)) @@ -77,8 +81,6 @@ static const size_t timeo_optlen = sizeof(timeo_sec); int connect_to_fd(int family, int type, int server_fd) { - struct sockaddr_storage addr; - socklen_t len = sizeof(addr); int fd; fd = socket(family, type, 0); @@ -87,24 +89,64 @@ int connect_to_fd(int family, int type, int server_fd) return -1; } - if (setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &timeo_sec, timeo_optlen)) { + if (connect_fd_to_fd(fd, server_fd) < 0 && errno != EINPROGRESS) { + close(fd); + return -1; + } + + return fd; +} + +int connect_fd_to_fd(int client_fd, int server_fd) +{ + struct sockaddr_storage addr; + socklen_t len = sizeof(addr); + + if (setsockopt(client_fd, SOL_SOCKET, SO_RCVTIMEO, &timeo_sec, + timeo_optlen)) { log_err("Failed to set SO_RCVTIMEO"); - goto out; + return -1; } if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) { log_err("Failed to get server addr"); - goto out; + return -1; } - if (connect(fd, (const struct sockaddr *)&addr, len) < 0) { - log_err("Fail to connect to server with family %d", family); - goto out; + if (connect(client_fd, (const struct sockaddr *)&addr, len) < 0) { + if (errno != EINPROGRESS) + log_err("Failed to connect to server"); + return -1; } - return fd; + return 0; +} + +int connect_wait(int fd) +{ + struct epoll_event ev = {}, events[2]; + int timeout_ms = 1000; + int efd, nfd; + + efd = epoll_create1(EPOLL_CLOEXEC); + if (efd < 0) { + log_err("Failed to open epoll fd"); + return -1; + } + + ev.events = EPOLLRDHUP | EPOLLOUT; + ev.data.fd = fd; + + if (epoll_ctl(efd, EPOLL_CTL_ADD, fd, &ev) < 0) { + log_err("Failed to register fd=%d on epoll fd=%d", fd, efd); + close(efd); + return -1; + } + + nfd = epoll_wait(efd, events, ARRAY_SIZE(events), timeout_ms); + if (nfd < 0) + log_err("Failed to wait for I/O event on epoll fd=%d", efd); -out: - close(fd); - return -1; + close(efd); + return nfd; } diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h index a0be7db4f67d..86914e6e7b53 100644 --- a/tools/testing/selftests/bpf/network_helpers.h +++ b/tools/testing/selftests/bpf/network_helpers.h @@ -35,5 +35,7 @@ extern struct ipv6_packet pkt_v6; int start_server(int family, int type); int connect_to_fd(int family, int type, int server_fd); +int connect_fd_to_fd(int client_fd, int server_fd); +int connect_wait(int client_fd); #endif
Add two new network helpers. connect_fd_to_fd connects an already created client socket fd to address of server fd. Sometimes it's useful to separate client socket creation and connecting this socket to a server, e.g. if client socket has to be created in a cgroup different from that of server cgroup. Additionally connect_to_fd is now implemented using connect_fd_to_fd, both helpers don't treat EINPROGRESS as an error and let caller decide how to proceed with it. connect_wait is a helper to work with non-blocking client sockets so that if connect_to_fd or connect_fd_to_fd returned -1 with errno == EINPROGRESS, caller can wait for connect to finish or for connection timeout. The helper returns -1 on error, 0 on timeout (1sec, hard-coded), and positive number on success. Signed-off-by: Andrey Ignatov <rdna@fb.com> --- tools/testing/selftests/bpf/network_helpers.c | 66 +++++++++++++++---- tools/testing/selftests/bpf/network_helpers.h | 2 + 2 files changed, 56 insertions(+), 12 deletions(-)