Re: [PATCH mptcp-next v6 3/7] mptcp: pm: improve error messages

[Geliang Tang <geliang@kernel.org>]

On Mon, 2024-12-30 at 14:24 +0100, Matthieu Baerts (NGI0) wrote:
> Some error messages were:
> 
>  - too generic: "missing input", "invalid request"
> 
>  - not precise enough: "limit greater than maximum" but what's the
> max?
> 
>  - missing: subflow not found, or connect error.
> 
> This can be easily improved by being more precise, or adding new
> error
> messages.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/pm_netlink.c   | 7 ++++---
>  net/mptcp/pm_userspace.c | 9 ++++++++-
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index
> 49c34494e156ff6c3277b107c3840ee29dd3afbb..3bb213e2a967e87d4244dc0f1ba
> 58d3ba21fb371 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1878,8 +1878,9 @@ static int parse_limit(struct genl_info *info,
> int id, unsigned int *limit)
>  
>  	*limit = nla_get_u32(attr);
>  	if (*limit > MPTCP_PM_ADDR_MAX) {
> -		NL_SET_ERR_MSG_ATTR(info->extack, attr,
> -				    "limit greater than maximum");
> +		NL_SET_ERR_MSG_ATTR_FMT(info->extack, attr,
> +					"limit greater than maximum
> (%u)",
> +					MPTCP_PM_ADDR_MAX);
>  		return -EINVAL;
>  	}
>  	return 0;
> @@ -2008,7 +2009,7 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb,
> struct genl_info *info)
>  		lookup_by_id = 1;
>  		if (!addr.addr.id) {
>  			NL_SET_ERR_MSG_ATTR(info->extack, attr,
> -					    "missing required
> inputs");
> +					    "missing address ID");
>  			return -EOPNOTSUPP;
>  		}
>  	}
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index
> 9c0bec588c498e0aa78acf4018db509abe90cad2..a8f73b082460ba00f7fa998cf76
> 46368f8201e2e 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -191,7 +191,7 @@ static struct mptcp_sock
> *mptcp_userspace_pm_get_sock(const struct genl_info *in
>  
>  	if (!mptcp_pm_is_userspace(msk)) {
>  		NL_SET_ERR_MSG_ATTR(info->extack, token,
> -				    "invalid request; userspace PM
> not selected");
> +				    "userspace PM not selected");
>  		sock_put((struct sock *)msk);
>  		return NULL;
>  	}
> @@ -433,6 +433,9 @@ int mptcp_pm_nl_subflow_create_doit(struct
> sk_buff *skb, struct genl_info *info)
>  	err = __mptcp_subflow_connect(sk, &local, &addr_r);
>  	release_sock(sk);
>  
> +	if (err)
> +		GENL_SET_ERR_MSG_FMT(info, "connect error: %d",
> err);
> +
>  	spin_lock_bh(&msk->pm.lock);
>  	if (err)
>  		mptcp_userspace_pm_delete_local_addr(msk, &entry);
> @@ -557,6 +560,7 @@ int mptcp_pm_nl_subflow_destroy_doit(struct
> sk_buff *skb, struct genl_info *info
>  	lock_sock(sk);
>  	ssk = mptcp_nl_find_ssk(msk, &addr_l.addr, &addr_r);
>  	if (!ssk) {
> +		GENL_SET_ERR_MSG(info, "subflow not found");
>  		err = -ESRCH;
>  		goto release_sock;
>  	}
> @@ -634,6 +638,9 @@ int mptcp_userspace_pm_set_flags(struct sk_buff
> *skb, struct genl_info *info)
>  	ret = mptcp_pm_nl_mp_prio_send_ack(msk, &loc.addr,
> &rem.addr, bkup);
>  	release_sock(sk);
>  
> +	if (ret)
> +		GENL_SET_ERR_MSG(info, "subflow not found");

I guess you don't want to use "subflow not found" here. I changed it as
"mp_prio send ack failed" in v7.

> +
>  set_flags_err:
>  	sock_put(sk);
>  	return ret;
>