| Message ID | 20241230-genl_req_attr_check-v6-3-3ec9103559e7@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geliang Tang |
| Headers | show |
| Series | mptcp: use GENL_REQ_ATTR_CHECK in userspace pm | expand |
| Context | Check | Description |
|---|---|---|
| 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 |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf-normal__only_bpftest_all_ | success | Success! ✅ |
| matttbe/KVM_Validation__btf-debug__only_bpftest_all_ | warning | Unstable: 1 failed test(s): bpftest_test_progs-no_alu32_mptcp |
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; >
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
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
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;
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(-)