netdev - RE: [PATCH][next] tipc: remove redundant assignment to ret, simplify code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <AS4PR05MB964780C1F48B61844D518C6B88052@AS4PR05MB9647.eurprd05.prod.outlook.com>
Date: Thu, 11 Apr 2024 11:42:29 +0000
From: Tung Quang Nguyen <tung.q.nguyen@...tech.com.au>
To: Dan Carpenter <dan.carpenter@...aro.org>
CC: "Colin King (gmail)" <colin.i.king@...il.com>, Jon Maloy
	<jmaloy@...hat.com>, Ying Xue <ying.xue@...driver.com>, "David S . Miller"
	<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski
	<kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "tipc-discussion@...ts.sourceforge.net"
	<tipc-discussion@...ts.sourceforge.net>, "kernel-janitors@...r.kernel.org"
	<kernel-janitors@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH][next] tipc: remove redundant assignment to ret, simplify
 code

>On Thu, Apr 11, 2024 at 11:04:15AM +0000, Tung Quang Nguyen wrote:
>> >Subject: Re: [PATCH][next] tipc: remove redundant assignment to ret,
>> >simplify code
>> >
>> >On 11/04/2024 11:31, Dan Carpenter wrote:
>> >> On Thu, Apr 11, 2024 at 10:04:10AM +0000, Tung Quang Nguyen wrote:
>> >>>>
>> >>> I suggest that err variable should be completely removed. Could
>> >>> you please also do the same thing for this code ?
>> >>> "
>> >>> ...
>> >>> err = skb_handler(skb, cb, tsk);
>> >>> if (err) {
>> >>
>> >> If we write the code as:
>> >>
>> >> 	if (some_function(parameters)) {
>> >>
>> >> then at first that looks like a boolean.  People probably think the
>> >> function returns true/false.  But if we leave it as-is:
>> >>
>> >> 	err = some_function(parameters);
>> >> 	if (err) {
>> >>
>> >> Then that looks like error handling.
>> >>
>> >> So it's better and more readable to leave it as-is.
>> >>
>> >> regards,
>> >> dan carpenter
>> >
>> >I concur with Dan's comments.
>> >
>> >Colin
>> I have a different view.
>> It does not make sense to me to use stack variable 'err' just for
>> checking return code of the functions (__tipc_nl_add_sk/
>> __tipc_add_sock_diag) that we know always return true on error.
>>
>
>I think you are trying to mirco optimize the code at the expense of readability.  It is unnecessary.  
I do not see any issue with readability when doing so.

>The compiler is smart enough to
>generate the same code either way.  I have just tested this on my system and it is true.
>
Yeah, I do not deny that. The obvious thing I can see is using err is a redundant thing.

>$ md5sum net/tipc/socket.o.*
>f5ebea97eeb9736c5b8097158c2b12e5  net/tipc/socket.o.without_var
>f5ebea97eeb9736c5b8097158c2b12e5  net/tipc/socket.o.with_var $
>
>When you're doing these tests, you need to ensure that the line numbers do change so I have commented out the old lines instead of
>deleting them.
>
>regards,
>dan carpenter
>
>diff --git a/net/tipc/socket.c b/net/tipc/socket.c index 7e4135db5816..879a8a9786b0 100644
>--- a/net/tipc/socket.c
>+++ b/net/tipc/socket.c
>@@ -3560,24 +3560,21 @@ int tipc_nl_sk_walk(struct sk_buff *skb, struct netlink_callback *cb,  {
> 	struct rhashtable_iter *iter = (void *)cb->args[4];
> 	struct tipc_sock *tsk;
>-	int err;
>+//	int err;
>
> 	rhashtable_walk_start(iter);
> 	while ((tsk = rhashtable_walk_next(iter)) != NULL) {
> 		if (IS_ERR(tsk)) {
>-			err = PTR_ERR(tsk);
>-			if (err == -EAGAIN) {
>-				err = 0;
>+			if (PTR_ERR(tsk) == -EAGAIN)
> 				continue;
>-			}
> 			break;
> 		}
>
> 		sock_hold(&tsk->sk);
> 		rhashtable_walk_stop(iter);
> 		lock_sock(&tsk->sk);
>-		err = skb_handler(skb, cb, tsk);
>-		if (err) {
>+//		err = skb_handler(skb, cb, tsk);
>+		if (skb_handler(skb, cb, tsk)) {
> 			release_sock(&tsk->sk);
> 			sock_put(&tsk->sk);
> 			goto out;
>