[PATCH] sched/net/act: Remove temporary state variables

[PATCH] sched/net/act: Remove temporary state variables

[Li zeming]

The temporary variable ret could be removed and the corresponding state
can be directly returned.

Signed-off-by: Li zeming <zeming@nfschina.com>
---
 net/sched/act_api.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 817065aa2833..34b5eb52e68b 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -398,8 +398,6 @@ static int __tcf_action_put(struct tc_action *p, bool bind)
 
 static int __tcf_idr_release(struct tc_action *p, bool bind, bool strict)
 {
-	int ret = 0;
-
 	/* Release with strict==1 and bind==0 is only called through act API
 	 * interface (classifiers always bind). Only case when action with
 	 * positive reference count and zero bind count can exist is when it was
@@ -417,10 +415,10 @@ static int __tcf_idr_release(struct tc_action *p, bool bind, bool strict)
 			return -EPERM;
 
 		if (__tcf_action_put(p, bind))
-			ret = ACT_P_DELETED;
+			return ACT_P_DELETED;
 	}
 
-	return ret;
+	return 0;
 }
 
 int tcf_idr_release(struct tc_action *a, bool bind)
-- 
2.18.2

Re: [PATCH] sched/net/act: Remove temporary state variables

[Jakub Kicinski]

On Wed, 27 Jul 2022 17:41:46 +0800 Li zeming wrote:
> The temporary variable ret could be removed and the corresponding state
> can be directly returned.

How many case like this are there in the kernel?
What tool are you using to find this?
We should focus on creating CI tools which can help catch instances of
this pattern in new code before it gets added, rather than cleaning up
old code. It just makes backports harder for hardly any gain.

Re: [PATCH] sched/net/act: Remove temporary state variables

[Jiri Pirko]

Fri, Jul 29, 2022 at 05:15:56AM CEST, kuba@kernel.org wrote:
>On Wed, 27 Jul 2022 17:41:46 +0800 Li zeming wrote:
>> The temporary variable ret could be removed and the corresponding state
>> can be directly returned.
>
>How many case like this are there in the kernel?
>What tool are you using to find this?
>We should focus on creating CI tools which can help catch instances of
>this pattern in new code before it gets added, rather than cleaning up
>old code. It just makes backports harder for hardly any gain.

What backports do you have in mind exactly?

Re: [PATCH] sched/net/act: Remove temporary state variables

[Jakub Kicinski]

On Fri, 29 Jul 2022 08:30:35 +0200 Jiri Pirko wrote:
>> How many case like this are there in the kernel?
>> What tool are you using to find this?
>> We should focus on creating CI tools which can help catch instances of
>> this pattern in new code before it gets added, rather than cleaning up
>> old code. It just makes backports harder for hardly any gain.  
> 
> What backports do you have in mind exactly?

Code backports. I don't understand the question.
There's little benefit and we're getting multiple such patches a day.

Re: [PATCH] sched/net/act: Remove temporary state variables

[Jiri Pirko]

Fri, Jul 29, 2022 at 08:51:21AM CEST, kuba@kernel.org wrote:
>On Fri, 29 Jul 2022 08:30:35 +0200 Jiri Pirko wrote:
>>> How many case like this are there in the kernel?
>>> What tool are you using to find this?
>>> We should focus on creating CI tools which can help catch instances of
>>> this pattern in new code before it gets added, rather than cleaning up
>>> old code. It just makes backports harder for hardly any gain.  
>> 
>> What backports do you have in mind exactly?
>
>Code backports. I don't understand the question.

Code backports of what where?
Are you talking about:
1) mainline kernels
2) distrubutions kernels? Or even worse, in-house kernels of companies?

If 2), I believe it is not relevant for the upstream discussion, at all.



>There's little benefit and we're getting multiple such patches a day.

Re: [PATCH] sched/net/act: Remove temporary state variables

[Jakub Kicinski]

On Fri, 29 Jul 2022 09:00:07 +0200 Jiri Pirko wrote:
> >> What backports do you have in mind exactly?  
> >
> >Code backports. I don't understand the question.  
> 
> Code backports of what where?
> Are you talking about:
> 1) mainline kernels
> 2) distrubutions kernels? Or even worse, in-house kernels of companies?
> 
> If 2), I believe it is not relevant for the upstream discussion, at all.

Fixes and stable. Frankly it's just a generic justification 
to discourage people from sending subjective code cleanups.
I'd never argue for the benefit of (2) :)

There's been a string of patches cleaning up return values
of functions in the last few days. If people have a lot of
time on their hands they should go do something useful, like
converting netdev features to a bitmap. Hell, go fix W=1 warnings, 
even easier.

The time spent reviewing those "cleanups" adds up, and I suspect
there's hundreds of places they can be applied. Hence my question
about automation... 

Re: [PATCH] sched/net/act: Remove temporary state variables

[Jiri Pirko]

Fri, Jul 29, 2022 at 09:18:42AM CEST, kuba@kernel.org wrote:
>On Fri, 29 Jul 2022 09:00:07 +0200 Jiri Pirko wrote:
>> >> What backports do you have in mind exactly?  
>> >
>> >Code backports. I don't understand the question.  
>> 
>> Code backports of what where?
>> Are you talking about:
>> 1) mainline kernels
>> 2) distrubutions kernels? Or even worse, in-house kernels of companies?
>> 
>> If 2), I believe it is not relevant for the upstream discussion, at all.
>
>Fixes and stable. Frankly it's just a generic justification 

Was there a significant value of breakages for net and stable backports
in past?


>to discourage people from sending subjective code cleanups.
>I'd never argue for the benefit of (2) :)

Uff, for a second, it did sound like it :)


>
>There's been a string of patches cleaning up return values
>of functions in the last few days. If people have a lot of

Well, I think it is good to send a patch to clean something up when you
spot it. If you don't do it, someone else might do it again in the
future anyway.

Plus there is one good reason at least to do this kinds of cleanups.
People tend to copy&paste code without thinking twice. So if you clean
it up here, it might not get copied into other code. That's good.


>time on their hands they should go do something useful, like
>converting netdev features to a bitmap. Hell, go fix W=1 warnings, 
>even easier.

Random spot&clean is hardly comparable to this.


>
>The time spent reviewing those "cleanups" adds up, and I suspect
>there's hundreds of places they can be applied. Hence my question
>about automation...