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