Re: [PATCH bpf v6] selftests/bpf: allocate a larger timeout for connection

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index b82f572641b7..1fda1fa7281c 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c

[ ... ]

> @@ -291,6 +293,37 @@ int client_socket(int family, int type,
>  	return -1;
>  }
>
> +static int connect_with_timeout(int fd, const struct sockaddr_storage *addr,
> +				socklen_t addrlen, int timeout_ms)
> +{
> +	int connect_timeout_ms = MAX(timeout_ms, CONNECT_MIN_TIMEOUT_MS);
> +
> +	/*
> +	 * Override timeout configuration with a larger value for the
> +	 * connection
> +	 */
> +	if (settimeo(fd, connect_timeout_ms)) {
> +		log_err("Failed to set connect timeout");
> +		return -1;
> +	}
> +
> +	if (connect(fd, (const struct sockaddr *)addr, addrlen)) {
> +		log_err("Failed to connect");
> +		return -1;
> +	}
> +
> +	/*
> +	 * If the timeout configured by the test is different from the
> +	 * connect timeout, restore it
> +	 */

This isn't a bug, but do the two block comments here add anything over
the code they sit above? The first sits above settimeo(fd,
connect_timeout_ms) where connect_timeout_ms was computed with MAX(...,
CONNECT_MIN_TIMEOUT_MS) on the line before. The second transliterates
the condition on the line below (timeout_ms != connect_timeout_ms means
"restore it"). Could they be trimmed to a single line about why a larger
connect budget is needed?

> +	if (timeout_ms != connect_timeout_ms && settimeo(fd, timeout_ms)) {
> +		log_err("Failed to set timeout for connected socket");
> +		return -1;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> @@ -376,20 +408,14 @@ int connect_fd_to_fd(int client_fd, int server_fd, int timeout_ms)
>  	struct sockaddr_storage addr;
>  	socklen_t len = sizeof(addr);
>
> -	if (settimeo(client_fd, timeout_ms))
> -		return -1;
> -
>  	if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
>  		log_err("Failed to get server addr");
>  		return -1;
>  	}
>
> -	if (connect(client_fd, (const struct sockaddr *)&addr, len)) {
> -		log_err("Failed to connect to server");
> -		return -1;
> -	}
> -
> -	return 0;
> +	return connect_with_timeout(client_fd,
> +				    &addr, len,
> +				    timeout_ms);
                            ^^^^

This isn't a bug, but could this call fit on one line? With the leading
tab it comes to about 71 columns as a single statement, well inside the
80-column limit. Elsewhere the file only wraps arguments when the line
doesn't fit.

>  }


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32062499118