| Message ID | 20240411091704.306752-1-colin.i.king@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 195b7fc53c6f45feb0f649c7c68af70900928253 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [next] tipc: remove redundant assignment to ret, simplify code | expand |
>--- 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) { ... } ... "
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
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
>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.
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;
>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; >
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!
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; }
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(-)