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