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

[bot+bpf-ci@kernel.org]

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

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

[ ... ]

> @@ -305,13 +313,33 @@ int connect_to_addr(int type, const struct sockaddr_storage *addr, socklen_t add
>  		return -1;
>  	}
>
> +	/* Override timeout configuration with a larger value for the
> +	 * connection
> +	 */
> +	if (settimeo(fd, connect_timeout_ms(opts))) {

A subsystem pattern flags this as potentially concerning:

Both multi-line comments added by this change start their text on the
same line as the opening '/*'. The loaded BPF subsystem guide requires
the opening '/*' to sit on its own line for files under
tools/testing/selftests/bpf/.

The guide specifies: "Multi-line comments MUST have the opening /* on
its own line, with the comment text starting on the next line."

However, this file contains 8 pre-existing multi-line comments (at
lines 455, 896, 1064, 1070, 1076, 1110, 1181, 1286) and every one puts
text on the opening '/*' line; there are zero instances of the form the
guide prescribes. Across tools/testing/selftests/bpf/ the ratio is
roughly 1986 to 689 in favour of the style used here.

Should these comments match the guide's requirement, or is the local
convention the right choice for this file?

> +		log_err("Failed to set connect timeout");
> +		goto close;
> +	}
> +
>  	if (connect(fd, (const struct sockaddr *)addr, addrlen)) {
> -		log_err("Failed to connect to server");
> -		save_errno_close(fd);
> -		return -1;
> +		log_err("Failed to connect");
> +		goto close;
> +	}
> +
> +	/* If the timeout configured by the test is different from the
> +	 * connect timeout, restore it
> +	 */
> +	if (opts->timeout_ms != CONNECT_MIN_TIMEOUT_MS &&
> +	    settimeo(fd, opts->timeout_ms)) {
> +		log_err("Failed to set timeout for connected socket");
> +		goto close;
> 	}
>
>  	return fd;
> +
> +close:
> +	save_errno_close(fd);
> +	return -1;
>  }


---
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/31688637156