Re: [PATCH mptcp-next v6 4/7] mptcp: pm: userspace: flags: clearer msg if no remote addr

[Geliang Tang <geliang@kernel.org>]

On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
> Since its introduction in commit 892f396c8e68 ("mptcp: netlink: issue
> MP_PRIO signals from userspace PMs"), it was mandatory to specify the
> remote address, because of the 'if (rem->addr.family == AF_UNSPEC)'
> check done later one.
> 
> In theory, this attribute can be optional, but it sounds better to be
> precise to avoid sending the MP_PRIO on the wrong subflow, e.g. if
> there
> are multiple subflows attached to the same local ID. This can be
> relaxed
> later on if there is a need to act on multiple subflows with one
> command.
> 
> For the moment, the check to see if attr_rem is NULL can be removed,
> because mptcp_pm_parse_entry() will do this check as well, no need to
> do
> that differently here.
> 
> While at it, move the parsing after the check linked to the local
> attribute.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/pm_userspace.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index
> a8f73b082460ba00f7fa998cf7646368f8201e2e..2f82ac49db35ec34dcc0b9208f4
> ac12edc6ab254 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -601,12 +601,6 @@ int mptcp_userspace_pm_set_flags(struct sk_buff
> *skb, struct genl_info *info)
>  	if (ret < 0)
>  		goto set_flags_err;
>  
> -	if (attr_rem) {
> -		ret = mptcp_pm_parse_entry(attr_rem, info, false,
> &rem);
> -		if (ret < 0)
> -			goto set_flags_err;
> -	}

I did not remove this 'if (attr_rem)' check in v7. I moved this code to
a common place for userspace PM and in-kernel PM. For in-kernel PM,
this check is required.

> -
>  	if (loc.addr.family == AF_UNSPEC) {
>  		NL_SET_ERR_MSG_ATTR(info->extack, attr,
>  				    "invalid local address family");
> @@ -614,6 +608,10 @@ int mptcp_userspace_pm_set_flags(struct sk_buff
> *skb, struct genl_info *info)
>  		goto set_flags_err;
>  	}
>  
> +	ret = mptcp_pm_parse_entry(attr_rem, info, false, &rem);
> +	if (ret < 0)
> +		goto set_flags_err;
> +
>  	if (rem.addr.family == AF_UNSPEC) {
>  		NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
>  				    "invalid remote address
> family");
>