Re: [PATCH mptcp-next v7 01/11] mptcp: pm: more precise error messages

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 06/01/2025 09:16, Geliang Tang wrote:
> From: "Matthieu Baerts (NGI0)" <matttbe@kernel.org>
> 
> Some errors reported by the userspace PM were vague: "this or that is
> invalid".
> 
> It is easier for the userspace to know which part is wrong, instead of
> having to guess that.
> 
> By splitting some error messages, NL_SET_ERR_MSG_ATTR() can be used
> instead of GENL_SET_ERR_MSG() in order to give an additional hint to the
> userspace developers about which attribute is wrong.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

If you modify the behaviour of a patch, you can add your 'co-dev' +
'sob' tags.

Also, please add an individual changelog, easier for the reviewers to
spot what has changed.

> ---
>  net/mptcp/pm_userspace.c | 39 +++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index a3d477059b11..40fd2f788196 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c

(...)

> @@ -579,17 +591,24 @@ int mptcp_userspace_pm_set_flags(struct sk_buff *skb, struct genl_info *info)
>  	if (ret < 0)
>  		goto set_flags_err;
>  
> +	if (loc.addr.family == AF_UNSPEC) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, attr,
> +				    "invalid local address family");
> +		ret = -EINVAL;
> +		goto set_flags_err;
> +	}
> +
>  	if (attr_rem) {
>  		ret = mptcp_pm_parse_entry(attr_rem, info, false, &rem);
>  		if (ret < 0)
>  			goto set_flags_err;
> -	}
>  
> -	if (loc.addr.family == AF_UNSPEC ||
> -	    rem.addr.family == AF_UNSPEC) {
> -		GENL_SET_ERR_MSG(info, "invalid address families");
> -		ret = -EINVAL;
> -		goto set_flags_err;
> +		if (rem.addr.family == AF_UNSPEC) {

As I mentioned in a previous version, this is changing the behaviour:
this remote attribute is no longer mandatory. I'm still unsure about
making it optional, because mptcp_pm_nl_mp_prio_send_ack() will pick
only one subflow, not all the ones with the specified local address. It
looks better to keep it mandatory.

At least here, you should not move it under "if (attr_rem)".

(and maybe it makes more sense to have this patch after 07/11 ("mptcp:
pm: use NL_SET_ERR_MSG_ATTR when possible"), but that's a detail, fine here)

> +			NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
> +					    "invalid remote address family");
> +			ret = -EINVAL;
> +			goto set_flags_err;
> +		}
>  	}
>  
>  	if (loc.flags & MPTCP_PM_ADDR_FLAG_BACKUP)

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