[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

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: 回复: [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