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@xxxxxxxxxx>
> 发送时间: 2023年7月7日 1:14
> 收件人: 王明-软件底层技术部 <machel@xxxxxxxx>
> 抄送: Jon Maloy <jmaloy@xxxxxxxxxx>; Ying Xue <ying.xue@xxxxxxxxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; tipc-discussion@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; opensource.kernel <opensource.kernel@xxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>
> 主题: Re: [PATCH v1] net:tipc:Remove repeated initialization
>
> [你通常不会收到来自 christophe.jaillet@xxxxxxxxxx 的电子邮件。请访问 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@xxxxxxxx>
> >
> > 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