Re: [PATCH bpf-next v6 1/9] selftests/bpf: Add backlog for network_helper_opts

[Martin KaFai Lau <martin.lau@xxxxxxxxx>]

On 6/28/24 6:20 PM, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@xxxxxxxxxx>
>
> Some callers expect __start_server() helper to pass their own "backlog"
> value to listen() instead of the default of 1. So this patch adds struct
> member "backlog" for network_helper_opts to allow callers to set "backlog"
> value via start_server_str() helper.
>
> Signed-off-by: Geliang Tang <tanggeliang@xxxxxxxxxx>
> ---
>   tools/testing/selftests/bpf/network_helpers.c | 2 +-
>   tools/testing/selftests/bpf/network_helpers.h | 1 +
>   2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/network_helpers.c b/tools/testing/selftests/bpf/network_helpers.c
> index 44c2c8fa542a..16cbb3fdcabf 100644
> --- a/tools/testing/selftests/bpf/network_helpers.c
> +++ b/tools/testing/selftests/bpf/network_helpers.c
> @@ -106,7 +106,7 @@ static int __start_server(int type, const struct sockaddr *addr, socklen_t addrl
>   	}
>   
>   	if (type == SOCK_STREAM) {
> -		if (listen(fd, 1) < 0) {
> +		if (listen(fd, opts->backlog ? : 1) < 0) {


iirc, listen(fd, 0 /* backlog */) can be used to enforce syncookie. Meaning 
backlog 0 is a legit value.

Using 0 as a default and changing it to 1 here is fine. It makes the test 
program easier to write for the common case. Enforcing syncookie mode by using 
backlog 0 is a niche use case but it should at least have a way for the caller 
to do that.

May be using -ve backlog to do that, like

	if (listen(fd, opts->backlog ? max(opts->backlog, 0): 1) < 0)


>   			log_err("Failed to listed on socket");
>   			goto error_close;
>   		}
> diff --git a/tools/testing/selftests/bpf/network_helpers.h b/tools/testing/selftests/bpf/network_helpers.h
> index 9ea36524b9db..8339c4e4b075 100644
> --- a/tools/testing/selftests/bpf/network_helpers.h
> +++ b/tools/testing/selftests/bpf/network_helpers.h
> @@ -25,6 +25,7 @@ struct network_helper_opts {
>   	int timeout_ms;
>   	bool must_fail;
>   	int proto;
> +	int backlog;

and add a comment here to explain the meaning of the -ve backlog.

>   	int (*post_socket_cb)(int fd, void *opts);
>   	void *cb_opts;
>   };