| Message ID | 27b15c2de6fe36088ef95c4823d9e4a6d7bc9ec6.1742898970.git.tanggeliang@kylinos.cn (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-next,v1] mptcp: fix cast type of is_fully_established | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| 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 25/03/2025 11:36, Geliang Tang wrote: > From: Geliang Tang <tanggeliang@kylinos.cn> > > The parameter type of mptcp_is_fully_established() is 'struct sock *', > not 'void *', so this patch casts 'msk' as 'struct sock *' instead of > 'void *' in mptcp_can_accept_new_subflow(). Indeed, but it is not really wrong to use 'void *'. Also, it looks like we are doing that in most other files from the net/mptcp directory. If you have to modify this line for another commit, feel free to include the modification in this commit if it bothers you ("While at it, use the proper cast instead of the generic 'void *'"), but it can also be left unchanged I think. Cheers, Matt
Hi Geliang,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/14057787415
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/8313b14b74fa
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=947109
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hi Matt, Thanks for the review. On Tue, 2025-03-25 at 11:49 +0100, Matthieu Baerts wrote: > Hi Geliang, > > On 25/03/2025 11:36, Geliang Tang wrote: > > From: Geliang Tang <tanggeliang@kylinos.cn> > > > > The parameter type of mptcp_is_fully_established() is 'struct sock > > *', > > not 'void *', so this patch casts 'msk' as 'struct sock *' instead > > of > > 'void *' in mptcp_can_accept_new_subflow(). > > Indeed, but it is not really wrong to use 'void *'. Also, it looks > like > we are doing that in most other files from the net/mptcp directory. I just checked, and there're indeed 6 other places that use '(void *)msk' too, but more places (98) use '(struct sock *)msk'. $ grep -r "(void \*)msk" net/mptcp/ | wc -l 7 $ grep -r "(struct sock \*)msk" net/mptcp/ | wc -l 98 I'm wondering if we should change all 7 '(void *)msk' to '(struct sock *)msk' in v2 to be consistent. > > If you have to modify this line for another commit, feel free to > include > the modification in this commit if it bothers you ("While at it, use > the > proper cast instead of the generic 'void *'"), but it can also be > left > unchanged I think. Or simply drop this patch. Both are OK to me. Thanks, -Geliang > > Cheers, > Matt
Hi Geliang, On 26/03/2025 02:08, Geliang Tang wrote: > Hi Matt, > > Thanks for the review. > > On Tue, 2025-03-25 at 11:49 +0100, Matthieu Baerts wrote: >> Hi Geliang, >> >> On 25/03/2025 11:36, Geliang Tang wrote: >>> From: Geliang Tang <tanggeliang@kylinos.cn> >>> >>> The parameter type of mptcp_is_fully_established() is 'struct sock >>> *', >>> not 'void *', so this patch casts 'msk' as 'struct sock *' instead >>> of >>> 'void *' in mptcp_can_accept_new_subflow(). >> >> Indeed, but it is not really wrong to use 'void *'. Also, it looks >> like >> we are doing that in most other files from the net/mptcp directory. > > I just checked, and there're indeed 6 other places that use '(void > *)msk' too, but more places (98) use '(struct sock *)msk'. > > $ grep -r "(void \*)msk" net/mptcp/ | wc -l > 7 > $ grep -r "(struct sock \*)msk" net/mptcp/ | wc -l > 98 > > I'm wondering if we should change all 7 '(void *)msk' to '(struct sock > *)msk' in v2 to be consistent. Maybe, I don't know to be honest. There are other (void *) usages, and they are "fine" for the moment. Changing them might be good for the consistency, but annoying in case of backports :-/ >> If you have to modify this line for another commit, feel free to >> include >> the modification in this commit if it bothers you ("While at it, use >> the >> proper cast instead of the generic 'void *'"), but it can also be >> left >> unchanged I think. > > Or simply drop this patch. Both are OK to me. Yes, maybe. I will ask Mat what he thinks about that. Cheers, Matt
On Wed, 2025-03-26 at 11:17 +0100, Matthieu Baerts wrote: > Hi Geliang, > > On 26/03/2025 02:08, Geliang Tang wrote: > > Hi Matt, > > > > Thanks for the review. > > > > On Tue, 2025-03-25 at 11:49 +0100, Matthieu Baerts wrote: > > > Hi Geliang, > > > > > > On 25/03/2025 11:36, Geliang Tang wrote: > > > > From: Geliang Tang <tanggeliang@kylinos.cn> > > > > > > > > The parameter type of mptcp_is_fully_established() is 'struct > > > > sock > > > > *', > > > > not 'void *', so this patch casts 'msk' as 'struct sock *' > > > > instead > > > > of > > > > 'void *' in mptcp_can_accept_new_subflow(). > > > > > > Indeed, but it is not really wrong to use 'void *'. Also, it > > > looks > > > like > > > we are doing that in most other files from the net/mptcp > > > directory. > > > > I just checked, and there're indeed 6 other places that use '(void > > *)msk' too, but more places (98) use '(struct sock *)msk'. > > > > $ grep -r "(void \*)msk" net/mptcp/ | wc -l > > 7 > > $ grep -r "(struct sock \*)msk" net/mptcp/ | wc -l > > 98 > > > > I'm wondering if we should change all 7 '(void *)msk' to '(struct > > sock > > *)msk' in v2 to be consistent. > > Maybe, I don't know to be honest. There are other (void *) usages, > and > they are "fine" for the moment. Changing them might be good for the > consistency, but annoying in case of backports :-/ > > > > If you have to modify this line for another commit, feel free to > > > include > > > the modification in this commit if it bothers you ("While at it, > > > use > > > the > > > proper cast instead of the generic 'void *'"), but it can also be > > > left > > > unchanged I think. > > > > Or simply drop this patch. Both are OK to me. > > Yes, maybe. I will ask Mat what he thinks about that. No need, let's drop this patch :) Thanks, -Geliang > > Cheers, > Matt
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 409bd415ef1d..da452a1d5433 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -60,7 +60,7 @@ static void subflow_generate_hmac(u64 key1, u64 key2, u32 nonce1, u32 nonce2, static bool mptcp_can_accept_new_subflow(const struct mptcp_sock *msk) { - return mptcp_is_fully_established((void *)msk) && + return mptcp_is_fully_established((struct sock *)msk) && ((mptcp_pm_is_userspace(msk) && mptcp_userspace_pm_active(msk)) || READ_ONCE(msk->pm.accept_subflow));