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..3bb213e2a967e87d4244dc0f1ba58d3ba21fb371 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..a8f73b082460ba00f7fa998cf7646368f8201e2e 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");
+
set_flags_err:
sock_put(sk);
return ret;
--
2.47.1On 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;
>
Hi Geliang, On 06/01/2025 09:41, Geliang Tang wrote: > 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. (...) >> 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 (...) >> @@ -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. No, I wanted to use "subflow not found": mptcp_pm_nl_mp_prio_send_ack() will fail only if it cannot find the subflow matching the local and remote attributes. Cheers, Matt -- Sponsored by the NGI0 Core fund.
Happy New Year Matt! On Mon, 2025-01-06 at 09:45 +0100, Matthieu Baerts wrote: > Hi Geliang, > > On 06/01/2025 09:41, Geliang Tang wrote: > > 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. > > (...) > > > > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c > > > index > > > 9c0bec588c498e0aa78acf4018db509abe90cad2..a8f73b082460ba00f7fa998 > > > cf76 > > > 46368f8201e2e 100644 > > > --- a/net/mptcp/pm_userspace.c > > > +++ b/net/mptcp/pm_userspace.c > > (...) > > > > @@ -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. > > No, I wanted to use "subflow not found": > mptcp_pm_nl_mp_prio_send_ack() > will fail only if it cannot find the subflow matching the local and > remote attributes. My bad, how about using "subflow not found in set_flags" here, and use "subflow not found in subflow_destroy" for mptcp_pm_nl_subflow_destroy_doit? > > Cheers, > Matt
On 06/01/2025 09:55, Geliang Tang wrote: > Happy New Year Matt! Thank you! :) Happy New Year as well (even if I should probably say that to you at the end of January :) ) > On Mon, 2025-01-06 at 09:45 +0100, Matthieu Baerts wrote: >> Hi Geliang, >> >> On 06/01/2025 09:41, Geliang Tang wrote: >>> 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. >> >> (...) >> >>>> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c >>>> index >>>> 9c0bec588c498e0aa78acf4018db509abe90cad2..a8f73b082460ba00f7fa998 >>>> cf76 >>>> 46368f8201e2e 100644 >>>> --- a/net/mptcp/pm_userspace.c >>>> +++ b/net/mptcp/pm_userspace.c >> >> (...) >> >>>> @@ -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. >> >> No, I wanted to use "subflow not found": >> mptcp_pm_nl_mp_prio_send_ack() >> will fail only if it cannot find the subflow matching the local and >> remote attributes. > > My bad, how about using "subflow not found in set_flags" here, and use > "subflow not found in subflow_destroy" for > mptcp_pm_nl_subflow_destroy_doit? I don't think we need a different error message: the userspace will send a specific Netlink command, and an error can be returned. No need to mention which command it is I think. If you prefer, feel free to add a comment above this error here: /* mptcp_pm_nl_mp_prio_send_ack will fail if it cannot find a subflow */ While at it, if you plan to send a v8, please use 'if (ret < 0)' to clearly show we are looking at errors. (When the variable is called 'err', that's clearer, but no need to change it here.) Cheers, Matt -- Sponsored by the NGI0 Core fund.
© 2016 - 2026 Red Hat, Inc.