cpupower: Remove redundant error check
Subsystems (2)
PATCH 2/4 cpupower: Remove redundant error check
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.
--- tools/power/cpupower/utils/cpufreq-set.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
tools/power/cpupower/utils/cpufreq-set.c +3 -4
@@ -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; }
tools/power/cpupower/utils/cpufreq-set.c int cmd_freq_set(int argc, char **argv) · line 325
Quoted 12 lines · Peter Senna Tschudin
tools/power/cpupower/utils/cpufreq-set.c +2
-1@@ -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;
tools/power/cpupower/utils/cpufreq-set.c int cmd_freq_set(int argc, char **argv) · line 329
Quoted 7 lines
tools/power/cpupower/utils/cpufreq-set.c +1
-3+ } } -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
Quoted 24 lines · Dan Carpenter
tools/power/cpupower/utils/cpufreq-set.c int cmd_freq_set(int argc, char **argv) · line 325
Quoted 12 lines · Peter Senna Tschudin
tools/power/cpupower/utils/cpufreq-set.c +2
-1@@ -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;
tools/power/cpupower/utils/cpufreq-set.c int cmd_freq_set(int argc, char **argv) · line 329
Quoted 7 lines
tools/power/cpupower/utils/cpufreq-set.c +1
-3+ } } -if (ret) -print_error (); -return ret;
Quoted 7 lines
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?
Quoted 4 lines
regards,
dan carpenter
Signature · 1 line
Peter
Quoted 30 lines · Peter Senna Tschudin
Quoted 28 lines · Dan Carpenter
tools/power/cpupower/utils/cpufreq-set.c int cmd_freq_set(int argc, char **argv) · line 325
Quoted 12 lines · Peter Senna Tschudin
tools/power/cpupower/utils/cpufreq-set.c +2
-1@@ -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;
tools/power/cpupower/utils/cpufreq-set.c int cmd_freq_set(int argc, char **argv) · line 329
Quoted 7 lines
tools/power/cpupower/utils/cpufreq-set.c +1
-3+ } } -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.
Quoted 7 lines
Quoted 3 lines
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
Quoted 45 lines · Dan Carpenter
Quoted 30 lines · Peter Senna Tschudin
Quoted 28 lines · Dan Carpenter
tools/power/cpupower/utils/cpufreq-set.c int cmd_freq_set(int argc, char **argv) · line 325
Quoted 12 lines · Peter Senna Tschudin
tools/power/cpupower/utils/cpufreq-set.c +2
-1@@ -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;
tools/power/cpupower/utils/cpufreq-set.c int cmd_freq_set(int argc, char **argv) · line 329
Quoted 7 lines
tools/power/cpupower/utils/cpufreq-set.c +1
-3+ } } -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.Quoted 7 lines
Quoted 3 lines
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:
Quoted 8 lines
ret = do_one_cpu(cpu, &new_pol, freq, policychange);
if (ret) {
print_error();
return ret;
}
}
return 0;Sent V2. Thank you for the help.
Quoted 4 lines
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?
Quoted 4 lines
regards,
dan carpenter
Signature · 1 line
Peter
Quoted 8 lines · Peter Senna Tschudin
Quoted 4 lines
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.
Quoted 4 lines
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