Re: [PATCH mptcp-next v3 9/9] mptcp: drop useless "err = 0" in subflow_destroy

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 07/11/2024 07:45, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Upon successful return, mptcp_pm_parse_addr() returns 0. There is no need
> to set "err = 0" after this. So after mptcp_nl_find_ssk() returns, just
> need to set "err = -ESRCH", then release and free msk socket if it returns
> NULL.
> 
> Also, no need to define the veriable "subflow" in subflow_destroy(), use
> mptcp_subflow_ctx(ssk) directly.
> 
> This patch doesn't change the behaviour of the code, just refactoring.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm_userspace.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 07e0c7259494..8545212f023e 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -535,19 +535,18 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
>  
>  	lock_sock(sk);
>  	ssk = mptcp_nl_find_ssk(msk, &addr_l.addr, &addr_r);
> -	if (ssk) {
> -		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> -
> -		spin_lock_bh(&msk->pm.lock);
> -		mptcp_userspace_pm_delete_local_addr(msk, &addr_l);
> -		spin_unlock_bh(&msk->pm.lock);
> -		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
> -		mptcp_close_ssk(sk, ssk, subflow);
> -		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
> -		err = 0;

OK to drop 'err = 0' but...

> -	} else {
> +	if (!ssk) {
>  		err = -ESRCH;
> +		release_sock(sk);
> +		goto destroy_err;

... I think it is always best to reduce the number of exit path: so
here, I think it is best to add a new label before release_sock(sk)
below, instead of duplicating this release_sock(sk). Something like:

      ssk = mptcp_nl_find_ssk(...);
      if (!ssk) {
          err = -ESRCH;
          goto release_sock;
      }

      (...)

  release_sock:
      release_sock(sk);

  destroy_err:
      (...)

WDYT?

>  	}
> +
> +	spin_lock_bh(&msk->pm.lock);
> +	mptcp_userspace_pm_delete_local_addr(msk, &addr_l);
> +	spin_unlock_bh(&msk->pm.lock);
> +	mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
> +	mptcp_close_ssk(sk, ssk, mptcp_subflow_ctx(ssk));
> +	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
>  	release_sock(sk);
>  
>  destroy_err:

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