Re: [RFC PATCH mptcp-next v7 06/11] add mptcp_setsockopt_fastopen

[Matthieu Baerts <matthieu.baerts@tessares.net>]

Hi Dmytro,

On 18/09/2022 00:28, Dmytro Shytyi wrote:
> Add set MPTFO socket option for MPTCP.

It is often very helpful to not just mention what you are doing here
(e.g. add XXX) but explain the reason why you are doing this. Getting
the reason is often not clear when looking at a commit diff so it is a
very valuable info to add in a commit description.

For example here, it could be nice to explain this option is needed for
the listen socket only to accept TFO. And also why you are changing the
values directly, what it is similar to, etc. etc.

> 
> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>
> ---
>  net/mptcp/fastopen.c | 28 ++++++++++++++++++++++++++++
>  net/mptcp/protocol.h |  2 ++
>  net/mptcp/sockopt.c  |  3 +++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
> index 50b5c3376672..436e773d798a 100644
> --- a/net/mptcp/fastopen.c
> +++ b/net/mptcp/fastopen.c
> @@ -63,3 +63,31 @@ int mptcp_sendmsg_fastopen(struct sock *sk, struct msghdr *msg,
>  	ret = -EFAULT;
>  	return ret;
>  }
> +
> +int mptcp_setsockopt_sol_tcp_fastopen(struct mptcp_sock *msk, sockptr_t optval,
> +				      unsigned int optlen)
> +{
> +	struct sock *sk = (struct sock *)msk;
> +	struct net *net = sock_net(sk);
> +	int val;
> +	int ret;
> +
> +	ret = 0;
> +
> +	if (copy_from_sockptr(&val, optval, sizeof(val)))
> +		return -EFAULT;
> +
> +	lock_sock(sk);
> +
> +	if (val >= 0 && ((1 << sk->sk_state) & (TCPF_CLOSE |
> +	    TCPF_LISTEN))) {
> +		tcp_fastopen_init_key_once(net);
> +		fastopen_queue_tune(sk, val);

Can you not call tcp_setsockopt() here instead of copying what is done
in do_tcp_setsockopt()?

It would be simpler and easier to maintain I think.


Also, in general for the series, may you add 'mptcp:' prefix in the git
commit titles?

What is important too is to add comments if what you are doing is
similar to what is done in TCP, e.g. similar to XXX() in tcp_XXX.c? You
can add a short comment in the code and explain why you are doing that
and not call TCP functions in the git commit description for example.
That would help a lot reviewers and maintainers later.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net