Re: [PATCH mptcp-next v4 7/7] mptcp: add local parameter for set_flags

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 10/01/2025 08:36, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch updates the interfaces set_flags to reduce repetitive
> code, adds a new parameter 'local' for them. The local address is
> parsed in public helper mptcp_pm_nl_set_flags_doit(), then pass
> it to mptcp_pm_nl_set_flags() and mptcp_userspace_pm_set_flags().

(...)

> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 52fd9830b685..ec6afa5c3525 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1951,61 +1951,51 @@ static int mptcp_nl_set_flags(struct net *net,
>  	return ret;
>  }
>  
> -int mptcp_pm_nl_set_flags(struct genl_info *info)
> +int mptcp_pm_nl_set_flags(struct mptcp_pm_addr_entry *local,
> +			  struct genl_info *info)
>  {
> -	struct mptcp_pm_addr_entry addr = { .addr = { .family = AF_UNSPEC }, };
>  	u8 changed, mask = MPTCP_PM_ADDR_FLAG_BACKUP |
>  			   MPTCP_PM_ADDR_FLAG_FULLMESH;
>  	struct net *net = genl_info_net(info);
>  	struct mptcp_pm_addr_entry *entry;
>  	struct pm_nl_pernet *pernet;
> -	struct nlattr *attr;
>  	u8 lookup_by_id = 0;
>  	u8 bkup = 0;
> -	int ret;
> -
> -	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR))
> -		return -EINVAL;
>  
>  	pernet = pm_nl_get_pernet(net);
>  
> -	attr = info->attrs[MPTCP_PM_ATTR_ADDR];

As I mentioned in the "use GENL_REQ_ATTR_CHECK in userspace pm" series,
I think this 'attr' can be kept here -- because we still have access to
'info' -- ...

> -	ret = mptcp_pm_parse_entry(attr, info, false, &addr);
> -	if (ret < 0)
> -		return ret;
> -
> -	if (addr.addr.family == AF_UNSPEC) {
> +	if (local->addr.family == AF_UNSPEC) {
>  		lookup_by_id = 1;
> -		if (!addr.addr.id) {
> +		if (!local->addr.id) {
>  			GENL_SET_ERR_MSG(info, "missing address ID");

... and use NL_SET_ERR_MSG_ATTR() here and below.

That way, the code around the reporting of errors would be uniformed
with the rest, and that could still help userspace devs.

I can do the modifications when applying the patches of the other series
and this one here.

(...)

> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 4007f67cc7b1..431454578021 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -564,19 +564,18 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
>  	return err;
>  }
>  
> -int mptcp_userspace_pm_set_flags(struct genl_info *info)
> +int mptcp_userspace_pm_set_flags(struct mptcp_pm_addr_entry *local,
> +				 struct genl_info *info)
>  {
> -	struct mptcp_pm_addr_entry loc = { .addr = { .family = AF_UNSPEC }, };
>  	struct mptcp_addr_info rem = { .family = AF_UNSPEC, };
>  	struct mptcp_pm_addr_entry *entry;
> -	struct nlattr *attr, *attr_rem;
> +	struct nlattr *attr_rem;
>  	struct mptcp_sock *msk;
>  	int ret = -EINVAL;
>  	struct sock *sk;
>  	u8 bkup = 0;
>  
> -	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR) ||
> -	    GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR_REMOTE))
> +	if (GENL_REQ_ATTR_CHECK(info, MPTCP_PM_ATTR_ADDR_REMOTE))
>  		return ret;
>  
>  	msk = mptcp_userspace_pm_get_sock(info);
> @@ -585,12 +584,7 @@ int mptcp_userspace_pm_set_flags(struct genl_info *info)
>  
>  	sk = (struct sock *)msk;
>  
> -	attr = info->attrs[MPTCP_PM_ATTR_ADDR];

Same here.

> -	ret = mptcp_pm_parse_entry(attr, info, false, &loc);
> -	if (ret < 0)
> -		goto set_flags_err;
> -
> -	if (loc.addr.family == AF_UNSPEC) {
> +	if (local->addr.family == AF_UNSPEC) {
>  		GENL_SET_ERR_MSG(info, "invalid local address family");
>  		ret = -EINVAL;
>  		goto set_flags_err;
(...)

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