| Message ID | 40c12b3865d03d2a173d10538ea38f3b2c9745ca.1741258415.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | BPF path manager, part 5 | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 31 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_ | success | Success! ✅ |
Hi Geliang, On 06/03/2025 12:01, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > This patch adds a helper mptcp_pm_validate() to check whether required > ops are defined. It will be invoked in .validate of struct bpf_struct_ops. > > Currently mandatory ops of mptcp_pm_ops are get_local_id and get_priority. Because more ops will be added soon, it looks a bit strange to introduce this helper now, no? If you have to change something else in this series, maybe best to squash this in patch 2/12 ("mptcp: pm: define struct mptcp_pm_ops") but ... > > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn> > --- > net/mptcp/pm.c | 16 ++++++++++++++++ > net/mptcp/protocol.h | 1 + > 2 files changed, 17 insertions(+) > > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c > index 29bc903658f7..5d666b0891b3 100644 > --- a/net/mptcp/pm.c > +++ b/net/mptcp/pm.c > @@ -1045,8 +1045,24 @@ struct mptcp_pm_ops *mptcp_pm_find(const char *name) > return NULL; > } > > +int mptcp_pm_validate(struct mptcp_pm_ops *pm_ops) > +{ > + if (!pm_ops->get_local_id || !pm_ops->get_priority) { > + pr_err("%s does not implement required ops\n", pm_ops->name); > + return -EINVAL; > + } ... without this if-statement: this can be introduced when adding each mandatory ops. I think this will help better understanding that they are mandatory when they are being introduced (patch 7 and 8/12). WDYT? (Please then add something like this in the commit message of patch 2/12 after the squash:) mptcp_pm_validate() will be invoked in .validate of struct bpf_struct_ops. That's why this function is exported. Cheers, Matt
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c index 29bc903658f7..5d666b0891b3 100644 --- a/net/mptcp/pm.c +++ b/net/mptcp/pm.c @@ -1045,8 +1045,24 @@ struct mptcp_pm_ops *mptcp_pm_find(const char *name) return NULL; } +int mptcp_pm_validate(struct mptcp_pm_ops *pm_ops) +{ + if (!pm_ops->get_local_id || !pm_ops->get_priority) { + pr_err("%s does not implement required ops\n", pm_ops->name); + return -EINVAL; + } + + return 0; +} + int mptcp_pm_register(struct mptcp_pm_ops *pm_ops) { + int ret; + + ret = mptcp_pm_validate(pm_ops); + if (ret) + return ret; + spin_lock(&mptcp_pm_list_lock); if (mptcp_pm_find(pm_ops->name)) { spin_unlock(&mptcp_pm_list_lock); diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index d26e89d960a1..1ce6c22cb295 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -1059,6 +1059,7 @@ extern struct mptcp_pm_ops mptcp_pm_kernel; struct mptcp_pm_ops *mptcp_pm_find(const char *name); int mptcp_pm_register(struct mptcp_pm_ops *pm_ops); void mptcp_pm_unregister(struct mptcp_pm_ops *pm_ops); +int mptcp_pm_validate(struct mptcp_pm_ops *pm_ops); void mptcp_userspace_pm_free_local_addr_list(struct mptcp_sock *msk);