Re: [PATCH mptcp-next v5 8/9] mptcp: pm: drop get_local_id helpers

[Matthieu Baerts <matttbe@kernel.org>]

On 20/02/2025 03:57, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Now mptcp_pm_nl_get_local_id() and mptcp_userspace_pm_get_local_id()
> helpers can be dropped, and mptcp_pm_get_local_id() can directly invoke
> get_local_id() interface through "ops" of "msk->pm".
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm.c           | 6 +++---
>  net/mptcp/pm_netlink.c   | 4 ++--
>  net/mptcp/pm_userspace.c | 4 ++--
>  net/mptcp/protocol.h     | 4 ----
>  4 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 9d42aed440a0..d2cc93e21bee 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -438,10 +438,10 @@ int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc)
>  	skc_local.addr.id = 0;
>  	skc_local.flags = MPTCP_PM_ADDR_FLAG_IMPLICIT;
>  
> +	if (!msk->pm.ops || !msk->pm.ops->get_local_id)
Mmh, do we really need to check the "no pm ops" case? If we have to deal
with that, that might be annoying.

Should we not impose the implementation of get_local_id()?

> +		return -ENOTSUPP;
>  	mptcp_pm_param_set_contexts(&param, &skc_local, NULL);
> -	if (mptcp_pm_is_userspace(msk))
> -		return mptcp_userspace_pm_get_local_id(msk, &param);
> -	return mptcp_pm_nl_get_local_id(msk, &param);
> +	return msk->pm.ops->get_local_id(msk, &param);
>  }
>  
>  bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc)
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index e0f8754e261e..33b19ff7a313 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1139,8 +1139,8 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk,
>  	return err;
>  }
>  
> -int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
> -			     struct mptcp_pm_param *param)
> +static int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
> +				    struct mptcp_pm_param *param)
>  {
>  	struct mptcp_pm_addr_entry *skc = &param->entry;
>  	struct mptcp_pm_addr_entry *entry;
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 6e51335c8bc1..b233d8469a48 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -130,8 +130,8 @@ mptcp_userspace_pm_lookup_addr_by_id(struct mptcp_sock *msk, unsigned int id)
>  	return NULL;
>  }
>  
> -int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
> -				    struct mptcp_pm_param *param)
> +static int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
> +					   struct mptcp_pm_param *param)
>  {
>  	__be16 msk_sport =  ((struct inet_sock *)
>  			     inet_sk((struct sock *)msk))->inet_sport;
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index cddb919fc120..d06add105df5 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1129,10 +1129,6 @@ void mptcp_pm_param_set_contexts(struct mptcp_pm_param *param,
>  				 const struct mptcp_pm_addr_entry *entry,
>  				 const struct mptcp_addr_info *addr);
>  int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
> -int mptcp_pm_nl_get_local_id(struct mptcp_sock *msk,
> -			     struct mptcp_pm_param *param);
> -int mptcp_userspace_pm_get_local_id(struct mptcp_sock *msk,
> -				    struct mptcp_pm_param *param);
>  bool mptcp_pm_is_backup(struct mptcp_sock *msk, struct sock_common *skc);
>  bool mptcp_pm_nl_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);
>  bool mptcp_userspace_pm_is_backup(struct mptcp_sock *msk, struct mptcp_addr_info *skc);

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