diff mbox series

[mptcp-next,v8,4/8] mptcp: pm: improve error messages

Message ID e874dbaa4aea1b221d9becd0dcf14abda24ac29b.1736299989.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded
Commit b4609c1a21bce3a5e594b21aa418a9bd9a77ba03
Delegated to: Matthieu Baerts
Headers show
Series mptcp: use GENL_REQ_ATTR_CHECK in userspace pm | expand

Checks

Context Check Description
matttbe/KVM_Validation__normal fail Critical: Global Timeout ❌
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ success Success! ✅
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ success Success! ✅
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 52 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/build success Build and static analysis OK

Commit Message

Geliang Tang Jan. 8, 2025, 1:44 a.m. UTC 
From: "Matthieu Baerts (NGI0)" <matttbe@kernel.org>

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.

v2: add a comment in mptcp_userspace_pm_set_flags().

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/pm_netlink.c   |  6 ++++--
 net/mptcp/pm_userspace.c | 10 +++++++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

Comments

Matthieu Baerts Jan. 9, 2025, 3:23 p.m. UTC | #1 
Hi Geliang,

On 08/01/2025 02:44, Geliang Tang wrote:
> From: "Matthieu Baerts (NGI0)" <matttbe@kernel.org>
> 
> 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.
> 
> v2: add a comment in mptcp_userspace_pm_set_flags().

Thank you for this in-line comment, I find it very helpful for the
reviewers compared to a global one in the cover-letter where we don't
know exactly which patch has been modified.

Just one thing: can you move this changelog comment below the last SoB,
under a line with '---'? ...

> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

... so here:

  ---
  - v2: (...)

(or you can also use 'git notes edit' and the '--notes' option with
'send-email' or 'format-patch'. If you use it for the first time, you
will need to set 'notes.rewriteRef' config not to lose notes after a
rebase, see: https://stackoverflow.com/a/14601464)

Same in patch 7/8 and 8/8.

Cheers,
Matt
diff mbox series

Patch

diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
index 98ac73938bd8..a60217faf95d 100644
--- a/net/mptcp/pm_netlink.c
+++ b/net/mptcp/pm_netlink.c
@@ -1875,7 +1875,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) {
-		GENL_SET_ERR_MSG(info, "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;
@@ -2003,7 +2005,7 @@  int mptcp_pm_nl_set_flags(struct sk_buff *skb, struct genl_info *info)
 	if (addr.addr.family == AF_UNSPEC) {
 		lookup_by_id = 1;
 		if (!addr.addr.id) {
-			GENL_SET_ERR_MSG(info, "missing required inputs");
+			GENL_SET_ERR_MSG(info, "missing address ID");
 			return -EOPNOTSUPP;
 		}
 	}
diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 2cb42091bd08..f5372e0a0dd9 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -190,7 +190,7 @@  static struct mptcp_sock *mptcp_userspace_pm_get_sock(const struct genl_info *in
 	}
 
 	if (!mptcp_pm_is_userspace(msk)) {
-		GENL_SET_ERR_MSG(info, "invalid request; userspace PM not selected");
+		GENL_SET_ERR_MSG(info, "userspace PM not selected");
 		sock_put((struct sock *)msk);
 		return NULL;
 	}
@@ -428,6 +428,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);
@@ -552,6 +555,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;
 	}
@@ -627,6 +631,10 @@  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);
 
+	/* mptcp_pm_nl_mp_prio_send_ack() only fails in one case */
+	if (ret < 0)
+		GENL_SET_ERR_MSG(info, "subflow not found");
+
 set_flags_err:
 	sock_put(sk);
 	return ret;