[PATCH v1] net:tipc:Remove repeated initialization

[PATCH v1] net:tipc:Remove repeated initialization

[Wang Ming]

The original code initializes 'tmp' twice,
which causes duplicate initialization issue.
To fix this, we remove the second initialization
of 'tmp' and use 'parent' directly forsubsequent
operations.

Signed-off-by: Wang Ming <machel@vivo.com>
---
 net/tipc/group.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/tipc/group.c b/net/tipc/group.c
index 3e137d8c9d2f..b2f964f62c36 100644
--- a/net/tipc/group.c
+++ b/net/tipc/group.c
@@ -284,8 +284,6 @@ static int tipc_group_add_to_tree(struct tipc_group *grp,
 	n = &grp->members.rb_node;
 	while (*n) {
 		tmp = container_of(*n, struct tipc_member, tree_node);
-		parent = *n;
-		tmp = container_of(parent, struct tipc_member, tree_node);
 		nkey = (u64)tmp->node << 32 | tmp->port;
 		if (key < nkey)
 			n = &(*n)->rb_left;
-- 
2.25.1

Re: [PATCH v1] net:tipc:Remove repeated initialization

[Jakub Kicinski]

On Thu,  6 Jul 2023 21:42:09 +0800 Wang Ming wrote:
> The original code initializes 'tmp' twice,
> which causes duplicate initialization issue.
> To fix this, we remove the second initialization
> of 'tmp' and use 'parent' directly forsubsequent
> operations.
> 
> Signed-off-by: Wang Ming <machel@vivo.com>

Please stop sending the "remove repeated initialization" patches 
to networking, thanks.

Re: [PATCH v1] net:tipc:Remove repeated initialization

[Christophe JAILLET]

Le 06/07/2023 à 17:47, Jakub Kicinski a écrit :
> On Thu,  6 Jul 2023 21:42:09 +0800 Wang Ming wrote:
>> The original code initializes 'tmp' twice,
>> which causes duplicate initialization issue.
>> To fix this, we remove the second initialization
>> of 'tmp' and use 'parent' directly forsubsequent
>> operations.
>>
>> Signed-off-by: Wang Ming <machel@vivo.com>
> 
> Please stop sending the "remove repeated initialization" patches
> to networking, thanks.
> 
> 

The patch also looks just bogus, as 'parent' is now always NULL when:
    rb_link_node(&m->tree_node, parent, n);

is called after the while loop.

CJ

回复: [PATCH v1] net:tipc:Remove repeated initialization

[王明-软件底层技术部]

Hi CJ
: )
So what you're saying is there's no repeat initialization problem here.

-----邮件原件-----
发件人: Christophe JAILLET <christophe.jaillet@wanadoo.fr> 
发送时间: 2023年7月7日 1:14
收件人: 王明-软件底层技术部 <machel@vivo.com>
抄送: Jon Maloy <jmaloy@redhat.com>; Ying Xue <ying.xue@windriver.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com>; Jakub Kicinski <kuba@kernel.org>
主题: Re: [PATCH v1] net:tipc:Remove repeated initialization

[你通常不会收到来自 christophe.jaillet@wanadoo.fr 的电子邮件。请访问 https://aka.ms/LearnAboutSenderIdentification，以了解这一点为什么很重要]

Le 06/07/2023 à 17:47, Jakub Kicinski a écrit :
> On Thu,  6 Jul 2023 21:42:09 +0800 Wang Ming wrote:
>> The original code initializes 'tmp' twice, which causes duplicate 
>> initialization issue.
>> To fix this, we remove the second initialization of 'tmp' and use 
>> 'parent' directly forsubsequent operations.
>>
>> Signed-off-by: Wang Ming <machel@vivo.com>
>
> Please stop sending the "remove repeated initialization" patches to 
> networking, thanks.
>
>

The patch also looks just bogus, as 'parent' is now always NULL when:
    rb_link_node(&m->tree_node, parent, n);

is called after the while loop.

CJ

Re: 回复: [PATCH v1] net:tipc:Remove repeated initialization

[Christophe JAILLET]

Le 07/07/2023 à 03:24, 王明-软件底层技术部 a écrit :
> Hi CJ
> : )
> So what you're saying is there's no repeat initialization problem here.

Hi,

First, top posting is usually not the better way to answer email.


Anyway, in your patch, you have:
  	n = &grp->members.rb_node;
  	while (*n) {
  		tmp = container_of(*n, struct tipc_member, tree_node);
-		parent = *n;
-		tmp = container_of(parent, struct tipc_member, tree_node);
  		nkey = (u64)tmp->node << 32 | tmp->port;
  		if (key < nkey)
  			n = &(*n)->rb_left;

You are right, 'tmp' is initialized twice. It is even initialized twice, 
with the same walue.

But in your patch, you also remove "parent = *n;"
So now, 'parent' is never updated in the function, and when 
rb_link_node() is called after the while loop, it is NULL.

So your patch changed the behaviour of the code and looks broken.

Something like:
  	while (*n) {
  		tmp = container_of(*n, struct tipc_member, tree_node);
		parent = *n;
-		tmp = container_of(parent, struct tipc_member, tree_node);
  		nkey = (u64)tmp->node << 32 | tmp->port;
  		if (key < nkey)
  			n = &(*n)->rb_left;

would look good to me but, as said by Jakub in the thread, is really 
unlikely to be applied.

The risk of breaking code (as you un-intentionally did) is higher than 
the value of removing a redundant initialization.

Reviewing such patches also consume maintainers' time to check for such 
potential errors and sometimes it is safer to leave things as-is in 
order not to waste time and avoid potential troubles.

CJ


> 
> -----邮件原件-----
> 发件人: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> 发送时间: 2023年7月7日 1:14
> 收件人: 王明-软件底层技术部 <machel@vivo.com>
> 抄送: Jon Maloy <jmaloy@redhat.com>; Ying Xue <ying.xue@windriver.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com>; Jakub Kicinski <kuba@kernel.org>
> 主题: Re: [PATCH v1] net:tipc:Remove repeated initialization
> 
> [你通常不会收到来自 christophe.jaillet@wanadoo.fr 的电子邮件。请访问 https://aka.ms/LearnAboutSenderIdentification，以了解这一点为什么很重要]
> 
> Le 06/07/2023 à 17:47, Jakub Kicinski a écrit :
>> On Thu,  6 Jul 2023 21:42:09 +0800 Wang Ming wrote:
>>> The original code initializes 'tmp' twice, which causes duplicate
>>> initialization issue.
>>> To fix this, we remove the second initialization of 'tmp' and use
>>> 'parent' directly forsubsequent operations.
>>>
>>> Signed-off-by: Wang Ming <machel@vivo.com>
>>
>> Please stop sending the "remove repeated initialization" patches to
>> networking, thanks.
>>
>>
> 
> The patch also looks just bogus, as 'parent' is now always NULL when:
>      rb_link_node(&m->tree_node, parent, n);
> 
> is called after the while loop.
> 
> CJ

Re: net: tipc: Remove repeated “initialization” in tipc_group_add_to_tree()

[Markus Elfring]

> The risk of breaking code (as you un-intentionally did) is higher than
> the value of removing a redundant initialization.

I find that it would be nicer to perform only required data processing steps.
The application of variable assignments can occasionally be improved further.

The corresponding efforts grow for proper code review.

The discussed change possibilities might belong to the adjustment category
“code cleanup”.
Some contributors have got difficulties to integrate presented ideas.
Thus it seems that more attractive incentives need to be offered
for potentially desirable software updates.

Regards,
Markus

RE: net: tipc: Remove repeated “initialization” in tipc_group_add_to_tree()

[Tung Quang Nguyen]

> -               parent = *n;
> -               tmp = container_of(parent, struct tipc_member, tree_node);
>                 nkey = (u64)tmp->node << 32 | tmp->port;
>                 if (key < nkey)
>                         n = &(*n)->rb_left;
[RESEND]
Your patch breaks my test suite. You introduced a new bug which was pointed out by Christophe.
It is a redundant assignment to variable tmp as you pointed out.
I suggest that you repost this cleanup together with other patches that you see need to be refactored (I believe there are still many things like this in TIPC) in the future, or I can do what you suggested in my future cleanup.

回复: [PATCH v1] net:tipc:Remove repeated initialization

[王明-软件底层技术部]

Hi Jakub Kicinski
: )
I understand, but I am confused about whether my modification is wrong?

-----邮件原件-----
发件人: Jakub Kicinski <kuba@kernel.org> 
发送时间: 2023年7月6日 23:47
收件人: 王明-软件底层技术部 <machel@vivo.com>
抄送: Jon Maloy <jmaloy@redhat.com>; Ying Xue <ying.xue@windriver.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com>
主题: Re: [PATCH v1] net:tipc:Remove repeated initialization

[Some people who received this message don't often get email from kuba@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

On Thu,  6 Jul 2023 21:42:09 +0800 Wang Ming wrote:
> The original code initializes 'tmp' twice, which causes duplicate 
> initialization issue.
> To fix this, we remove the second initialization of 'tmp' and use 
> 'parent' directly forsubsequent operations.
>
> Signed-off-by: Wang Ming <machel@vivo.com>

Please stop sending the "remove repeated initialization" patches to networking, thanks.

Re: 回复: [PATCH v1] net:tipc:Remove repeated initialization

[Jakub Kicinski]

On Fri, 7 Jul 2023 01:21:52 +0000 王明-软件底层技术部 wrote:
> Hi Jakub Kicinski
> : )
> I understand, but I am confused about whether my modification is wrong?

The changes are so trivial they are not worth spending time on.

And you haven't read:
https://www.kernel.org/doc/html/next/process/index.html

Re: [PATCH] net: tipc: Remove repeated “initialization”

[Markus Elfring]

> The original code initializes 'tmp' twice,
> which causes duplicate initialization issue.

Is it more appropriate to refer to a repetition of questionable variable assignments?


> To fix this, we remove the second initialization
> of 'tmp' and use 'parent' directly forsubsequent
> operations.

* Would you like to avoid a typo in this sentence?

* Please choose a better imperative change suggestion.

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n94


…
> +++ b/net/tipc/group.c
> @@ -284,8 +284,6 @@ static int tipc_group_add_to_tree(struct tipc_group *grp,
>  	n = &grp->members.rb_node;
>  	while (*n) {
>  		tmp = container_of(*n, struct tipc_member, tree_node);
> -		parent = *n;
> -		tmp = container_of(parent, struct tipc_member, tree_node);
>  		nkey = (u64)tmp->node << 32 | tmp->port;
…

How does the proposed deletion fit to the function call “rb_link_node(&m->tree_node, parent, n)”
after the loop?
https://elixir.bootlin.com/linux/v6.4.2/source/net/tipc/group.c#L277

Regards,
Markus