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

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

[Colin Ian King]

Variable err is being assigned a zero value and it is never read
afterwards in either the break path or continue path, the assignment
is redundant and can be removed. With it removed, the if statement
can also be simplified.

Cleans up clang scan warning:
net/tipc/socket.c:3570:5: warning: Value stored to 'err' is never
read [deadcode.DeadStores]

Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
---
 net/tipc/socket.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 7e4135db5816..798397b6811e 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -3565,11 +3565,8 @@ int tipc_nl_sk_walk(struct sk_buff *skb, struct netlink_callback *cb,
 	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;
 		}
 
-- 
2.39.2

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

[Tung Quang Nguyen]

>--- a/net/tipc/socket.c
>+++ b/net/tipc/socket.c
>@@ -3565,11 +3565,8 @@ int tipc_nl_sk_walk(struct sk_buff *skb, struct netlink_callback *cb,
> 	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;
> 		}
>
>--
>2.39.2
>
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) {
...
}
...
"

Re: [PATCH][next] tipc: remove redundant assignment to ret, simplify code

[Dan Carpenter]

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

Re: [PATCH][next] tipc: remove redundant assignment to ret, simplify code

[Colin King (gmail)]

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

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

[Tung Quang Nguyen]

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

Re: [PATCH][next] tipc: remove redundant assignment to ret, simplify code

[Dan Carpenter]

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.  The compiler is smart enough to
generate the same code either way.  I have just tested this on my system
and it is true.

$ 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;

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

[Tung Quang Nguyen]

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

Re: [PATCH][next] tipc: remove redundant assignment to ret, simplify code

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 11 Apr 2024 10:17:04 +0100 you wrote:
> Variable err is being assigned a zero value and it is never read
> afterwards in either the break path or continue path, the assignment
> is redundant and can be removed. With it removed, the if statement
> can also be simplified.
> 
> Cleans up clang scan warning:
> net/tipc/socket.c:3570:5: warning: Value stored to 'err' is never
> read [deadcode.DeadStores]
> 
> [...]

Here is the summary with links:
  - [next] tipc: remove redundant assignment to ret, simplify code
    https://git.kernel.org/netdev/net-next/c/195b7fc53c6f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html