Re: [PATCH mptcp-next 1/2] Squash to "selftests/bpf: Add mptcp subflow subtest"

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 20/08/2024 10:44, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Close client_fd as Martin suggested.
> 
> Please update my email address of this patch squashed to as:
> 
>   From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> since BPF CI complained about it:
> 
> WARNING: From:/Signed-off-by: email address mismatch: 'From: Geliang Tang <geliang@kernel.org>' != 'Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>'

Good point, I will try not to forget about that!

> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  tools/testing/selftests/bpf/prog_tests/mptcp.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index 9d4e3c1d4e7b..69fdcb28249d 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -383,7 +383,7 @@ static void run_subflow(char *new)
>  {
>  	int server_fd, client_fd, err;
>  	char cc[TCP_CA_NAME_MAX];
> -	socklen_t len = sizeof(cc);
> +	socklen_t len;
>  
>  	server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1, 0);
>  	if (!ASSERT_GE(server_fd, 0, "start_mptcp_server"))
> @@ -393,9 +393,12 @@ static void run_subflow(char *new)
>  	if (!ASSERT_GE(client_fd, 0, "connect to fd"))
>  		goto fail;
>  
> +	len = sizeof(cc);
>  	err = getsockopt(server_fd, SOL_TCP, TCP_CONGESTION, cc, &len);
> -	if (!ASSERT_OK(err, "getsockopt(srv_fd, TCP_CONGESTION)"))
> +	if (!ASSERT_OK(err, "getsockopt(server_fd, TCP_CONGESTION)")) {
> +		close(client_fd);

Probably better to keep the traditional way of dealing with errors: by
having only one exit path. In other words, can you replace the 'goto
fail' here by another one: "goto close_client" (vs "goto close_server")?

>  		goto fail;
> +	}
>  
>  	send_byte(client_fd);
>  

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.