Re: [PATCH mptcp-next v6 2/7] mptcp: pm: more precise error messages

[Geliang Tang <geliang@kernel.org>]

On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
> 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>
> ---
>  net/mptcp/pm_userspace.c | 34 ++++++++++++++++++++++++++--------
>  1 file changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index
> 72d4861e497eef5a516fc1c3ec1659807ffedd53..9c0bec588c498e0aa78acf4018d
> b509abe90cad2 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -224,9 +224,14 @@ int mptcp_pm_nl_announce_doit(struct sk_buff
> *skb, struct genl_info *info)
>  		goto announce_err;
>  	}
>  
> -	if (addr_val.addr.id == 0 || !(addr_val.flags &
> MPTCP_PM_ADDR_FLAG_SIGNAL)) {
> -		NL_SET_ERR_MSG_ATTR(info->extack, addr,
> -				    "invalid addr id or flags");
> +	if (addr_val.addr.id == 0) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, addr, "invalid
> addr id");
> +		err = -EINVAL;
> +		goto announce_err;
> +	}
> +
> +	if (!(addr_val.flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, addr, "invalid
> addr flags");
>  		err = -EINVAL;
>  		goto announce_err;
>  	}
> @@ -537,8 +542,14 @@ int mptcp_pm_nl_subflow_destroy_doit(struct
> sk_buff *skb, struct genl_info *info
>  		goto destroy_err;
>  	}
>  
> -	if (!addr_l.addr.port || !addr_r.port) {
> -		GENL_SET_ERR_MSG(info, "missing local or remote
> port");
> +	if (!addr_l.addr.port) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, laddr, "missing
> local port");
> +		err = -EINVAL;
> +		goto destroy_err;
> +	}
> +
> +	if (!addr_r.port) {
> +		NL_SET_ERR_MSG_ATTR(info->extack, raddr, "missing
> remote port");
>  		err = -EINVAL;
>  		goto destroy_err;
>  	}
> @@ -592,9 +603,16 @@ int mptcp_userspace_pm_set_flags(struct sk_buff
> *skb, struct genl_info *info)
>  			goto set_flags_err;
>  	}
>  
> -	if (loc.addr.family == AF_UNSPEC ||
> -	    rem.addr.family == AF_UNSPEC) {
> -		GENL_SET_ERR_MSG(info, "invalid address families");
> +	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 (rem.addr.family == AF_UNSPEC) {

I moved this check into "if (attr_rem) {" block in v7.

> +		NL_SET_ERR_MSG_ATTR(info->extack, attr_rem,
> +				    "invalid remote address
> family");
>  		ret = -EINVAL;
>  		goto set_flags_err;
>  	}
>