Re: [PATCH bpf v2] selftests/bpf: keep polling connection that is still in progress

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] `log_err()` prints the wrong error string because `errno` is updated after the logging call in the asynchronous connection failure paths.
--

--- Patch [1]: [PATCH bpf v2] selftests/bpf: keep polling connection that is still in progress ---
commit 533fe7390970bae0c12d4d1a4c61075dda1b64ff
Author: Alexis Lothoré (eBPF Foundation) <[email protected]>

selftests/bpf: keep polling connection that is still in progress

This commit reduces flaky connection failures in CI by polling sockets that
initially return EINPROGRESS. It uses poll() to wait up to a few seconds
for the socket to become writable. If the connection completes, it checks
the final result via getsockopt() without requiring large timeout increases.

> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index b82f572641b7d..2f2719a28f026 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c

[ ... ]

> @@ -305,13 +309,42 @@ int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t add
>  		return -1;
>  	}
>  
> -	if (connect(fd, (const struct sockaddr *)addr, addrlen)) {
> +	err = connect(fd, (const struct sockaddr *)addr, addrlen);
> +	if (err && errno == EINPROGRESS) {
> +		struct pollfd pfd = { .fd = fd, .events = POLLOUT };
> +
> +		err = poll(&pfd, 1, CONNECTION_IN_PROGRESS_TIMEOUT_MS);
> +
> +		if (err <= 0) {
> +			if (err == 0) {
> +				log_err("Connection timeout");
> +				errno = ETIMEDOUT;

[Severity: Medium]
Will this log the wrong error string?

Because log_err() automatically incorporates the current errno into its
output, and errno is still set to EINPROGRESS here, this will log an
unexpected error message instead of ETIMEDOUT. Should errno be set to
ETIMEDOUT before calling log_err()?

> +			} else {
> +				log_err("Failed to poll connection status");
> +			}
> +			goto close;
> +		}
> +
> +		errlen = sizeof(err);
> +		if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &errlen) < 0) {
> +			log_err("Failed to getsockopt");
> +			goto close;
> +		}
> +
> +		if (err) {
> +			log_err("Eventually failed to connect to server");
> +			errno = err;

[Severity: Medium]
Does this code also suffer from the same issue?

Since errno is still EINPROGRESS when log_err() is called, this will print the
wrong error reason. Should errno be updated to the socket error in err before
calling log_err()?

> +			goto close;
> +		}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1