Re: [PATCH mptcp-next v7 2/4] selftests/bpf: Add getsockopt to inspect mptcp subflow

[Matthieu Baerts <matttbe@kernel.org>]

On 03/09/2024 11:11, Matthieu Baerts wrote:
> On 03/09/2024 10:04, Geliang Tang wrote:
>> From: Geliang Tang <tanggeliang@kylinos.cn>
>>
>> This patch adds a "cgroup/getsockopt" way to inspect the subflows of a
>> mptcp socket.
>>
>> mptcp_for_each_stubflow() and other helpers related to list_dentry are
>> added into progs/mptcp_bpf.h.
>>
>> Add an extra "cgroup/getsockopt" prog to walk the msk->conn_list and use
>> bpf_core_cast to cast a pointer to tcp_sock for readonly. It will allow
>> to inspect all the fields in a tcp_sock.
> 
> (...)
> 
>> diff --git a/tools/testing/selftests/bpf/progs/mptcp_subflow.c b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
>> index 2e28f4a215b5..1053a795eb43 100644
>> --- a/tools/testing/selftests/bpf/progs/mptcp_subflow.c
>> +++ b/tools/testing/selftests/bpf/progs/mptcp_subflow.c
>> @@ -4,6 +4,7 @@
>>  
>>  /* vmlinux.h, bpf_helpers.h and other 'define' */
>>  #include "bpf_tracing_net.h"
>> +#include "mptcp_bpf.h"
>>  
>>  char _license[] SEC("license") = "GPL";
>>  
>> @@ -57,3 +58,92 @@ int mptcp_subflow(struct bpf_sock_ops *skops)
>>  
>>  	return 1;
>>  }
>> +
>> +static int _check_getsockopt_subflow_mark(struct bpf_sock *sk)
>> +{
>> +	struct mptcp_subflow_context *subflow;
>> +	struct mptcp_sock *msk;
>> +	int i = 0;
>> +
>> +	if (sk->protocol != IPPROTO_MPTCP) {
>> +		bpf_printk("MPTCP Subflow: unexpected protocol %u", sk->protocol);
>> +		return -1;
>> +	}
> 
> Out of curiosity, why did you move this code here and in
> _check_getsockopt_subflow_cc()?
> 
> I'm just surprised because recently, you tried to reduce the amount of
> duplicated code :)

While at it: it sounds better to have unique error messages, to know
where was the error. Here, a few messages are the same, e.g. protocol,
number of subflows, sk is null. Do not hesitate to add something to
differentiate them: level & optname, a string prefix, the line number, etc.

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