mptcp: fix cast type of is_fully_established

[PATCH mptcp-next v1] mptcp: fix cast type of is_fully_established

Posted by Geliang Tang 1 week, 3 days ago

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().

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/subflow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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));
-- 
2.43.0

Re: [PATCH mptcp-next v1] mptcp: fix cast type of is_fully_established

Posted by Matthieu Baerts 1 week, 3 days ago

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v1] mptcp: fix cast type of is_fully_established

Posted by Geliang Tang 1 week, 2 days ago

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

Re: [PATCH mptcp-next v1] mptcp: fix cast type of is_fully_established

Posted by Matthieu Baerts 1 week, 2 days ago

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
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v1] mptcp: fix cast type of is_fully_established

Posted by Geliang Tang 1 week, 2 days ago

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