* [PATCH 2/4] cpupower: Remove redundant error check
@ 2014-05-17 18:22 Peter Senna Tschudin
2014-05-17 20:22 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Peter Senna Tschudin @ 2014-05-17 18:22 UTC (permalink / raw)
To: Dominik Brodowski
Cc: Peter Senna Tschudin, Thomas Renninger, linux-kernel,
Rafael J. Wysocki, Alan Cox, kernel-janitors
Remove double checks, and move the call to print_error to the
first check. The simplified version of the coccinelle semantic
patch that fixes this issue is as follows:
// <smpl>
@@
expression E; identifier pr; expression list es;
@@
for(...;...;...){
...
- if (E) break;
+ if (E){
+ pr(es);
+ break;
+ }
...
}
- if(E) pr(es);
// </smpl>
Untested.
Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
tools/power/cpupower/utils/cpufreq-set.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/tools/power/cpupower/utils/cpufreq-set.c b/tools/power/cpupower/utils/cpufreq-set.c
index a416de8..4e2f35a 100644
--- a/tools/power/cpupower/utils/cpufreq-set.c
+++ b/tools/power/cpupower/utils/cpufreq-set.c
@@ -320,12 +320,11 @@ int cmd_freq_set(int argc, char **argv)
printf(_("Setting cpu: %d\n"), cpu);
ret = do_one_cpu(cpu, &new_pol, freq, policychange);
- if (ret)
+ if (ret) {
+ print_error();
break;
+ }
}
- if (ret)
- print_error();
-
return ret;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/4] cpupower: Remove redundant error check 2014-05-17 18:22 [PATCH 2/4] cpupower: Remove redundant error check Peter Senna Tschudin @ 2014-05-17 20:22 ` Dan Carpenter 2014-05-17 21:34 ` Peter Senna Tschudin 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2014-05-17 20:22 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Dominik Brodowski, Thomas Renninger, linux-kernel, Rafael J. Wysocki, Alan Cox, kernel-janitors On Sat, May 17, 2014 at 08:22:58PM +0200, Peter Senna Tschudin wrote: > diff --git a/tools/power/cpupower/utils/cpufreq-set.c b/tools/power/cpupower/utils/cpufreq-set.c > index a416de8..4e2f35a 100644 > --- a/tools/power/cpupower/utils/cpufreq-set.c > +++ b/tools/power/cpupower/utils/cpufreq-set.c > @@ -320,12 +320,11 @@ int cmd_freq_set(int argc, char **argv) > > printf(_("Setting cpu: %d\n"), cpu); > ret = do_one_cpu(cpu, &new_pol, freq, policychange); > - if (ret) > + if (ret) { > + print_error(); > break; Just return directly instead of break return; > + } > } > > - if (ret) > - print_error(); > - > return ret; Are you sure this patch is correct? Theoretically, it's possible to reach the end of this function without going hitting the "ret = do_one_cpu(...);" assignment. Don't be fooled by the "int ret = 0;" initialization, that is a trick initialization to mislead the unwary. By the end of the do while loop then "ret" is always -1. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/4] cpupower: Remove redundant error check 2014-05-17 20:22 ` Dan Carpenter @ 2014-05-17 21:34 ` Peter Senna Tschudin 2014-05-17 21:56 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Peter Senna Tschudin @ 2014-05-17 21:34 UTC (permalink / raw) To: Dan Carpenter Cc: Dominik Brodowski, Thomas Renninger, linux-kernel@vger.kernel.org, Rafael J. Wysocki, Alan Cox, kernel-janitors On Sat, May 17, 2014 at 10:22 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Sat, May 17, 2014 at 08:22:58PM +0200, Peter Senna Tschudin wrote: >> diff --git a/tools/power/cpupower/utils/cpufreq-set.c b/tools/power/cpupower/utils/cpufreq-set.c >> index a416de8..4e2f35a 100644 >> --- a/tools/power/cpupower/utils/cpufreq-set.c >> +++ b/tools/power/cpupower/utils/cpufreq-set.c >> @@ -320,12 +320,11 @@ int cmd_freq_set(int argc, char **argv) >> >> printf(_("Setting cpu: %d\n"), cpu); >> ret = do_one_cpu(cpu, &new_pol, freq, policychange); >> - if (ret) >> + if (ret) { >> + print_error(); >> break; > > Just return directly instead of break return; > >> + } >> } >> >> - if (ret) >> - print_error(); >> - >> return ret; > > Are you sure this patch is correct? Theoretically, it's possible to > reach the end of this function without going hitting the > "ret = do_one_cpu(...);" assignment. > > Don't be fooled by the "int ret = 0;" initialization, that is a trick > initialization to mislead the unwary. By the end of the do while loop > then "ret" is always -1. I have missed that, thank you for pointing this out. This patch is wrong and should not be applied, please ignore it. Dan, should I just leave this file as it is? > > regards, > dan carpenter > -- Peter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/4] cpupower: Remove redundant error check 2014-05-17 21:34 ` Peter Senna Tschudin @ 2014-05-17 21:56 ` Dan Carpenter 2014-05-17 22:31 ` Peter Senna Tschudin 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2014-05-17 21:56 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Dominik Brodowski, Thomas Renninger, linux-kernel@vger.kernel.org, Rafael J. Wysocki, Alan Cox, kernel-janitors On Sat, May 17, 2014 at 11:34:46PM +0200, Peter Senna Tschudin wrote: > On Sat, May 17, 2014 at 10:22 PM, Dan Carpenter > <dan.carpenter@oracle.com> wrote: > > On Sat, May 17, 2014 at 08:22:58PM +0200, Peter Senna Tschudin wrote: > >> diff --git a/tools/power/cpupower/utils/cpufreq-set.c b/tools/power/cpupower/utils/cpufreq-set.c > >> index a416de8..4e2f35a 100644 > >> --- a/tools/power/cpupower/utils/cpufreq-set.c > >> +++ b/tools/power/cpupower/utils/cpufreq-set.c > >> @@ -320,12 +320,11 @@ int cmd_freq_set(int argc, char **argv) > >> > >> printf(_("Setting cpu: %d\n"), cpu); > >> ret = do_one_cpu(cpu, &new_pol, freq, policychange); > >> - if (ret) > >> + if (ret) { > >> + print_error(); > >> break; > > > > Just return directly instead of break return; > > > >> + } > >> } > >> > >> - if (ret) > >> - print_error(); > >> - > >> return ret; > > > > Are you sure this patch is correct? Theoretically, it's possible to > > reach the end of this function without going hitting the > > "ret = do_one_cpu(...);" assignment. > > > > Don't be fooled by the "int ret = 0;" initialization, that is a trick > > initialization to mislead the unwary. By the end of the do while loop > > then "ret" is always -1. > I have missed that, thank you for pointing this out. This patch is > wrong and should not be applied, please ignore it. > > Dan, should I just leave this file as it is? I think in reality we should always hit the "ret = do_one_cpu()" assignment. But your static analysis tool should say that we don't know that, so that's why I brought it up. My guess is that the original code is bad and we should say: ret = do_one_cpu(cpu, &new_pol, freq, policychange); if (ret) { print_error(); return ret; } } return 0; I am currently involved in a number of threads, not just yours, where I am encouraging people to replace ambiguous returns with "return 0;". This is my life now. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/4] cpupower: Remove redundant error check 2014-05-17 21:56 ` Dan Carpenter @ 2014-05-17 22:31 ` Peter Senna Tschudin 2014-05-17 22:42 ` Dan Carpenter 0 siblings, 1 reply; 6+ messages in thread From: Peter Senna Tschudin @ 2014-05-17 22:31 UTC (permalink / raw) To: Dan Carpenter Cc: Dominik Brodowski, Thomas Renninger, linux-kernel@vger.kernel.org, Rafael J. Wysocki, Alan Cox, kernel-janitors On Sat, May 17, 2014 at 11:56 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Sat, May 17, 2014 at 11:34:46PM +0200, Peter Senna Tschudin wrote: >> On Sat, May 17, 2014 at 10:22 PM, Dan Carpenter >> <dan.carpenter@oracle.com> wrote: >> > On Sat, May 17, 2014 at 08:22:58PM +0200, Peter Senna Tschudin wrote: >> >> diff --git a/tools/power/cpupower/utils/cpufreq-set.c b/tools/power/cpupower/utils/cpufreq-set.c >> >> index a416de8..4e2f35a 100644 >> >> --- a/tools/power/cpupower/utils/cpufreq-set.c >> >> +++ b/tools/power/cpupower/utils/cpufreq-set.c >> >> @@ -320,12 +320,11 @@ int cmd_freq_set(int argc, char **argv) >> >> >> >> printf(_("Setting cpu: %d\n"), cpu); >> >> ret = do_one_cpu(cpu, &new_pol, freq, policychange); >> >> - if (ret) >> >> + if (ret) { >> >> + print_error(); >> >> break; >> > >> > Just return directly instead of break return; >> > >> >> + } >> >> } >> >> >> >> - if (ret) >> >> - print_error(); >> >> - >> >> return ret; >> > >> > Are you sure this patch is correct? Theoretically, it's possible to >> > reach the end of this function without going hitting the >> > "ret = do_one_cpu(...);" assignment. >> > >> > Don't be fooled by the "int ret = 0;" initialization, that is a trick >> > initialization to mislead the unwary. By the end of the do while loop >> > then "ret" is always -1. >> I have missed that, thank you for pointing this out. This patch is >> wrong and should not be applied, please ignore it. >> >> Dan, should I just leave this file as it is? > > I think in reality we should always hit the "ret = do_one_cpu()" > assignment. But your static analysis tool should say that we don't know > that, so that's why I brought it up. > > My guess is that the original code is bad and we should say: > > ret = do_one_cpu(cpu, &new_pol, freq, policychange); > if (ret) { > print_error(); > return ret; > } > } > > return 0; Sent V2. Thank you for the help. > > I am currently involved in a number of threads, not just yours, where I > am encouraging people to replace ambiguous returns with "return 0;". > This is my life now. So maybe you like this list of 160 places in which the return variable is initialized and only used as parameter to return(The list look good, but I haven't reviewed all 160, so there could be problems): http://pastebin.com/5kAbCP2e Does it worth doing something about those trivial cases? Do you have more examples of ambiguous returns, so I can help you hunt them? > > regards, > dan carpenter > -- Peter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/4] cpupower: Remove redundant error check 2014-05-17 22:31 ` Peter Senna Tschudin @ 2014-05-17 22:42 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2014-05-17 22:42 UTC (permalink / raw) To: Peter Senna Tschudin Cc: Dominik Brodowski, Thomas Renninger, linux-kernel@vger.kernel.org, Rafael J. Wysocki, Alan Cox, kernel-janitors On Sun, May 18, 2014 at 12:31:29AM +0200, Peter Senna Tschudin wrote: > > > > I am currently involved in a number of threads, not just yours, where I > > am encouraging people to replace ambiguous returns with "return 0;". > > This is my life now. > So maybe you like this list of 160 places in which the return variable > is initialized and only used as parameter to return(The list look > good, but I haven't reviewed all 160, so there could be problems): > http://pastebin.com/5kAbCP2e Fantastic! :) These things are easy to review because if it's wrong then the compile will break. > > Does it worth doing something about those trivial cases? > > Do you have more examples of ambiguous returns, so I can help you hunt them? The main thing is what your check finds. If you know that ret is zero then return zero. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-17 22:43 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-17 18:22 [PATCH 2/4] cpupower: Remove redundant error check Peter Senna Tschudin 2014-05-17 20:22 ` Dan Carpenter 2014-05-17 21:34 ` Peter Senna Tschudin 2014-05-17 21:56 ` Dan Carpenter 2014-05-17 22:31 ` Peter Senna Tschudin 2014-05-17 22:42 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox