Remove unnecessary null pointer checks?

Hello,

I have tried the following short semantic patch out on a source file directory for Linux 3.13.1.

@Remove_unnecessary_pointer_checks@
expression x;
@@
-if (x)

    kfree(x);

Some update candidates were found. I imagine that this search pattern can be extended a bit more.
How do think about the applicability of corresponding changes for current kernel source code?
What will be a good way to discuss such fixes on other mailing lists eventually?

Regards,
Markus

Not sure what you mean by extended. There are indeed some other functions in Linux that perform NULL tests before doing anything, and thus they don't really need a null test around them. Actually, it could be possible to find such functions using Coccinelle.

On the other hand, I think that one should study the code carefully before making such a change. There are several ways to make error handling code. One is to have one label at the end of the function, to have all error cases jump to there, and then to have if tests to decide what should be done. Another way is to have lots of labels, and jump directly to the right place, without any tests.

I think that the second way is nicer. So in removing such an if, one can consider whether the code could be further improved by adding more labels, and jumping to the right place directly, rather tha executing unnecessary code.

If you want to propose Linux related fixes, you may want to look at the kernel janitors mailing list.

http://vger.kernel.org/vger-lists.html#kernel-janitors

julia

I have tried the following search pattern.

@Show_functions_with_input_pointer_validation@
identifier fun, x;
type t;
@@
*fun(..., t* x, ...)
{
...
if (!x) return;
...
}

Will your current tool version "1.0.0-rc20" find also the kfree() functions?
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/mm/slab.c#n3641

Should I add any variant of "unlikely(ZERO_OR_NULL_PTR(...))" to my filter pattern eventually?

Would it make sense to add the shown semantic patches to a specific script directory?
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/scripts/coccinelle

Regards,
Markus

If you want to match a call to ZERO_OR_NULL_PTR, then you need to include that in the semantic patch. Coccinelle does not expand macros, so if doesn't see the relation to a NULL test.

Not sure to understand what the "shown semantic patches" refers to. What you have proposed is a semntic patch that finds functions that test their argument for NULL and return if the test is satisfied. That gives the user some information, but the rules in the Linux kernel are for finding bugs or making transformations.

Personally, I rather like null tests, if they are really needed, ie not implied to be always false by the context, because they give some information, eg that the value can be null at this point, and it is important to take that into account when considerin the following operation. So I'm not very eager to go around removing them.

julia

I have tried a command like the following a moment ago.

elfring at Sonne:~/Projekte/Coccinelle/janitor> MY_PATTERN=find_input_pointer_validation2.cocci && cat $MY_PATTERN && echo '-----' && spatch --version && spatch --sp-file $MY_PATTERN /usr/src/linux-stable/mm/slab.c --recursive-includes -I /usr/src/linux-stable/include/linux -I /usr/src/linux-stable/include/uapi -I /usr/src/linux-stable/arch/ia64/include -I /usr/src/linux-stable/include/asm-generic -I /usr/src/linux-stable/include/asm-generic/bitops -I /usr/local/include/c++/4.8.2 -I /usr/local/include/c++/4.8.2/x86_64-unknown-linux-gnu -I /usr/local/include -I /usr/include
@Show_functions_with_input_pointer_validation@
identifier fun, x;
type t;
@@
*void fun(..., t* x, ...)
{
...
if (unlikely(ZERO_OR_NULL_PTR(x)))

     return;

...
}
-----
spatch version 1.0.0-rc20 with Python support and with PCRE support
init_defs_builtins: ...
(ONCE) TYPE: header trace/events/kmem.h not found

previous modification:
MINUS
According to environment 2:
Show_functions_with_input_pointer_validation.t -> const
void
Show_functions_with_input_pointer_validation.fun -> id kfree

current modification:
MINUS
According to environment 2:
Show_functions_with_input_pointer_validation.t -> const void Show_functions_with_input_pointer_validation.fun -> id kfree

Fatal error: exception Failure("Show_functions_with_input_pointer_validation:
already tagged token:
C code context
File "/usr/src/linux-stable/mm/slab.c", line 3650, column 11, charpos = 94972
around = 'const', whole content = void kfree(const void *objp)")

How can I achieve a better result here?

I try to collect all names for functions which handle passed null pointers in a known way. I hope that such a function list can be converted into a constraint for a metavariable in another semantic patch variant.

Regards,
Markus

[...]

I tried this small source code search out again.

elfring at Sonne:~/Projekte/Coccinelle/janitor> date \

So 26. Okt 07:02:18 CET 2014
@Show_functions_with_input_pointer_validation@
identifier fun, x;
type t;
@@
*void fun(..., t* x, ...)
{
...
if (unlikely(ZERO_OR_NULL_PTR(x)))

     return;

...
}
-----
spatch version 1.0.0-rc22 with Python support and with PCRE support
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/mm/slab.c
(ONCE) TYPE: header process.h not found
[...]
(ONCE) TYPE: header trace/events/kmem.h not found
(ONCE) already tagged but only removed, so safe
diff =
--- /usr/src/linux-stable/mm/slab.c
@@ -3598,7 +3598,6 @@ EXPORT_SYMBOL(kmem_cache_free);
* Don't free memory not originally allocated by kmalloc()
* or you will run into trouble.
*/
-void kfree(const void *objp)

 {
        struct kmem_cache *c;
        unsigned long flags;

Note: processing took 43.5s: /usr/src/linux-stable/mm/slab.c
starting: Common.group_assoc_bykey_eff
ending: Common.group_assoc_bykey_eff, 0.000005s
---------------------
profiling result
---------------------

Main total                               :     43.552 sec          1 count
Main.outfiles computation                :     43.490 sec          1 count
full_engine                              :     43.481 sec          1 count
C parsing                                :     17.765 sec        866 count
TOTAL                                    :     17.763 sec        866 count
HACK                                     :      6.017 sec       2001 count
C parsing.fix_cpp                        :      5.559 sec       1134 count
C parsing.tokens                         :      3.528 sec        867 count
LEXING                                   :      3.506 sec        866 count
Parsing: multi pass                      :      2.818 sec       2369 count
Parsing: 1st pass                        :      2.258 sec      42256 count
MACRO managment                          :      1.725 sec       2671 count
YACC                                     :      1.634 sec      40007 count
TAC.annotate_program                     :      0.983 sec        433 count
show_xxx                                 :      0.855 sec        310 count
C parsing.fix_define                     :      0.761 sec       1733 count
MACRO mgmt prep 2                        :      0.494 sec        866 count
C consistencycheck                       :      0.461 sec        866 count
C parsing.lookahead                      :      0.407 sec     455401 count
Common.full_charpos_to_pos_large         :      0.389 sec        867 count
flow                                     :      0.371 sec      20946 count
C parsing.mk_info_item                   :      0.361 sec      42256 count
C parsing.lex_ident                      :      0.314 sec     313227 count
Type_c.type_of_s                         :      0.234 sec      10649 count
fix_flow                                 :      0.154 sec      15138 count
TAC.add_binding                          :      0.075 sec      18856 count
MACRO mgmt prep 1                        :      0.064 sec        866 count
pre_engine                               :      0.036 sec          1 count
parse cocci                              :      0.032 sec          1 count
bigloop                                  :      0.026 sec          1 count
Show_functions_with_input_pointer_validation :      0.023 sec          1 count
process_a_ctl_a_env_a_toplevel           :      0.023 sec        300 count
mysat                                    :      0.022 sec        300 count
Common.info_from_charpos                 :      0.018 sec         82 count
Common.=~                                :      0.013 sec       9107 count
worth_trying                             :      0.009 sec          1 count
Common.full_charpos_to_pos               :      0.005 sec          2 count
TAC.typedef_fix                          :      0.005 sec      14621 count
C unparsing                              :      0.005 sec          2 count
asttoctl2                                :      0.003 sec          1 count
ctl                                      :      0.000 sec          3 count
Transformation3.transform                :      0.000 sec          1 count
get_glimpse_constants                    :      0.000 sec          1 count
Main.result analysis                     :      0.000 sec          1 count
check_duplicate                          :      0.000 sec          1 count
Common.group_assoc_bykey_eff             :      0.000 sec          1 count
C_vs_c                                   :      0.000 sec          1 count
merge_env                                :      0.000 sec          1 count
post_engine                              :      0.000 sec          1 count
Main.infiles computation                 :      0.000 sec          1 count

So 26. Okt 07:03:01 CET 2014

Do you find this analysis result interesting?

Regards,
Markus

I would like to show again that the version 1.0.0-rc22 works better than 1.0.0-rc20 for another use case.

Interesting ...

Does your feedback refer also to the message "(ONCE) already tagged but only removed, so safe" eventually?

I am curious how the shown profiling results will change over time because of software improvements.

Regards,
Markus

I would also like to try another source code search approach like the following.

elfring at Sonne:~/Projekte/Coccinelle/janitor> LINE='-----' && echo $LINE && PAT=list_input_parameter_validation1.cocci && cat $PAT && echo $LINE && SP=/usr/local/bin/spatch && $SP --version && $SP --sp-file $PAT /usr/src/linux-stable/fs/btrfs/extent_map.c
-----
@initialize:python@
@@
import sys
result = []
mark = ['"', '', '"']
delimiter = '|'

def store_positions(fun, typ, point, places):

    """Add source code positions to an internal list."""
    for place in places:
        fields = []
        fields.append(fun)

        mark[1] = typ
        fields.append(''.join(mark))

        fields.append(point)

        mark[1] = place.file.replace('"', '""')
        fields.append(''.join(mark))

        fields.append(place.line)
        fields.append(str(int(place.column) + 1))
        result.append(delimiter.join(fields))

@safety_check@
identifier work, input;
type data_type;
position pos;
@@
void work at pos(...,data_type input,...)
{
...
(
if (!input) return;
|
if (input) { ... }
|
if (input) { ... } else { ... }
|
switch (input) { case 0: return; ... }
)
...
}

@script:python collection depends on safety_check@ typ << safety_check.data_type;
fun << safety_check.work;
point << safety_check.input;
places << safety_check.pos;
@@
store_positions(fun, typ, point, places)

@finalize:python@
@@
if result:
result.insert(0, delimiter.join(("function", '"data type"', '"parameter"',
'"source file"', "line", "column")))
print("\r\n".join(result))
else:
sys.stderr.write("No result for this analysis!\n")
-----
spatch version 1.0.0-rc20 with Python support and with PCRE support
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/fs/btrfs/extent_map.c
No result for this analysis!

Why is the function "free_extent_map" not displayed here? Do I need to adjust my SmPL rule "safety_check" anyhow?

Regards,
Markus

Something goes wrong with the switch pattern. I would have to look into why. But I think that a switch is highly improbable for making such a test, so you could just drop it. You may also want to specify that input has pointer type, since you are actually looking for NULL tests, not zero tests.

julia

Thanks for your quick feedback.

I just try to make the discussed filter patterns as complete as possible.

This is an important implementation detail which I would like to generalise so that also functions like "btrfsic_process_written_block" will be safely found by further source code searches.
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/fs/btrfs/check-integrity.c?id=878a876b2e10888afe53766dcca33f723ae20edc#n1835

This one was found by the previous simple pattern for example despite I expected it to handle primarily pointer data types instead of an "unsigned int" in this case.

Regards,
Markus

As you like. As a general principle, I think it is better to work on fewer things at once. There are many reason why values may be null or 0, and if you lump too many of them together, you will end up with more information than is manageable. No bugs are involved here, you are only making the code simpler, and if you miss some possibilities for making the code simpler, it doesn't matter. You can just work on them later.

julia

I have got another idea for a bit of fine-tuning. I got the impression from the manual that the SmPL syntax element "singleton" might be applicable.

elfring at Sonne:~/Projekte/Coccinelle/janitor> LINE='-----' && echo $LINE && PAT=list_input_parameter_validation2.cocci && cat $PAT && echo $LINE && SP=/usr/local/bin/spatch && $SP --version && $SP --sp-file $PAT /usr/src/linux-stable/fs/btrfs/extent_map.c
-----

@initialize:python@
@@
import sys
result = []
mark = ['"', '', '"']
delimiter = '|'

def store_positions(fun, typ, point, places):

    """Add source code positions to an internal list."""
    for place in places:
        fields = []
        fields.append(fun)

        mark[1] = typ
        fields.append(''.join(mark))

        fields.append(point)

        mark[1] = place.file.replace('"', '""')
        fields.append(''.join(mark))

        fields.append(place.line)
        fields.append(str(int(place.column) + 1))
        result.append(delimiter.join(fields))

@safety_check@
identifier work, input;
type data_type;
position pos;
statement is, es;
@@
void work at pos(...,data_type input,...)
{
...
( if (!input) return;
| if (input) is
? else es
;
)
...
}

@script:python collection depends on safety_check@ typ << safety_check.data_type;
fun << safety_check.work;
point << safety_check.input;
places << safety_check.pos;
@@
store_positions(fun, typ, point, places)

@finalize:python@
@@
if result:
result.insert(0, delimiter.join(("function", '"data type"', '"parameter"',
'"source file"', "line", "column")))
print("\r\n".join(result))
else:
sys.stderr.write("No result for this analysis!\n")
-----
spatch version 1.0.0-rc20 with Python support and with PCRE support
init_defs_builtins: /usr/local/share/coccinelle/standard.h
warning: incompatible arity found on line 36
?safety_check:es
Fatal error: exception Failure("get_before_e: not supported")

Can this situation be improved?

Regards,
Markus

Please don't put all of this noise in your messages. The only thing that is interesting is the file on which you have run Coccinelle.

There is no need to put the ? else es. If you put if (intput) is else es then an isomorphism will consider the case where else es is not there.

Also, the semicolon after is/es does not look right. Notmally one does not put an extra semicolon after an if.

julia

Thanks for your explanation.

I get a "surprise" if I try out the following SmPL variant.

@safety_check@
identifier function, input;
type data_type;
position pos;
statement is, es;
@@
void function at pos(...,data_type input,...)
{
...
( if (!input) return;
| if (input) is else es
)
...
}

@script:python collection depends on safety_check@ typ << safety_check.data_type;
fun << safety_check.function;
point << safety_check.input;
places << safety_check.pos;
@@
store_positions(fun, typ, point, places)

Response:
875 883
Fatal error: exception Failure("scriptmeta: parse error:
= File "list_input_parameter_validation3.cocci", line 43, column 20, charpos = 875

    around = 'function', whole content = fun << safety_check.function;

Can such names be reused which are key words@other places?

Regards,
Markus

Apparently there is a problem in this case. I will make a note of it. In the meantime, just use another name.

julia

I get the following result for the adjusted search pattern.

void work at pos(...,data_type input,...)
{
...
( if (!input) return;
| if (input) is else es
)
...
}

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch --sp-file list_input_parameter_validation2.cocci /usr/src/linux-stable/arch/um/kernel/sysrq.c
...
function|"data type"|"parameter"|"source file"|line|column show_stack|"unsigned long *"|stack|"/usr/src/linux-stable/arch/um/kernel/sysrq.c"|67|6

This example does not fit to my expectation because it seems that the function implementation does not refer to the passed values.
Do you get any idea for this potential mismatch?

Regards,
Markus

I'm not sure, but I think it has to do with the fact that the entire function is an if where both branches leave the function. It seems to be considering that to be error-handling code. It does not require to find the complete pattern on execution paths that correspond to error-handling code. So it doesn't require to find the pattern at all.

In your case, I think you want to be completely sure that regardless of the execution path chosen, the test is performed. So you should put when string after the first ... (ie the one after the first { ).

julia

I must correct my conclusion here. One function parameter is actually used in the condition "!stack" while the second parameter is forwarded to other function calls.
Is this source code line 83 handled by a SmPL isomorphism?

I'm sorry. I do not really understand your suggestion.

Regards,
Markus

Sorry, I had been looking at the wrong function when I wrote my previous response.

Indeed, there are some uses of isomorphisms at work here. First the

if (input) is else es

is changed to

if (!input) es else is

and then that is changed to

if (!input) es

which matches the code at line 83.

I'm not sure what is the point of the if (input) is else es pattern. I would think you only want to find cases where the function does nothing until it tests input and aborts the function is the value is 0. So you only want the return case. Before it, you would want

... when != S

where S is a statement typed metavariable. That way the ... will only match declarations.

julia

Now it seems that I added it because of too few considerations for isomorphisms.

We have got different expectations about the amount of source code before the statement "return". A bit more fine-tuning with the SmPL construct "when" might be needed to achieve further constraints.

Regards,
Markus

I'm sorry that I overlooked information somehow there.

The implementations of the function "show_stack" can be nice source code examples for further fine-tuning of proposed filter patterns.
https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/arch/um/kernel/sysrq.c?id=8ed12fcc194d93c6a17714120a7027ee4d76a881#n67

Its interface contains two pointer parameters.

I can also adjust the search approach like the following.

@safety_check@
identifier work, input;
type data_type;
position pos;
statement is, es;
@@
void work at pos(...,data_type input,...)
{
...
( if (input) is
| if (likely(input)) is
)
else es
...
}

I guess that the interpretation of "nothing relevant" will need further considerations, won't it?

Not "only" this one ...

I have got a feeling for a need to introspect the else branch for useful properties. I am unsure about better analysis of the metavariable "es" with the semantic patch language.

Regards,
Markus

This is not allowed. Inside a disjunction you need to have complete terms.

I'm not sure to understand your goal. If the code currently has:

if (x != NULL)
call(x);

then the developer does not want to execute any of the code within call if x is NULL. If you just check that there is a NULL test on x somewhere within the definition of call, then that is not enough to ensure that nothing is executed within call. If you remove the NULL test, you could drastically change the behavior of the program.

julia

Thanks for your information. I guess that another adjustment will fit to the SmPL syntax then.

( if (input) is else es
| if (likely(input)) is else es
)

Yes. - This is one of the use cases I am trying to improve. It depends on the knowledge if something unwanted will happen if a null pointer (or zero) would be passed. If it is documented for example that an implementation of the function "call" checks the condition "!= NULL", I find the same check by the caller redundant.

Would you like to suggest any more fine-tuning?

I agree in principle.

But I would like to delete redundant checks from some source files.

Regards,
Markus

I get an interesting result for example if I try the following source code search pattern out. Do you find it useful?

@Delete_unnecessary_checks@
expression x;
identifier release =~ "^(?x)
(?:
(?:kz?|slob_)free
|
(?:
abc
| xyz
)
)$";
@@
-if (x)

    release(...,x,...);

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch --sp-file delete_unnecessary_checks1.cocci /usr/src/linux-stable/fs/btrfs/inode.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/fs/btrfs/inode.c

diff =
@@ -5137,8 +5137,7 @@ static int btrfs_dentry_delete(const str
 
 static void btrfs_dentry_release(struct dentry *dentry)
 {
-       if (dentry->d_fsdata)
-               kfree(dentry->d_fsdata);
+       kfree(dentry->d_fsdata);
 }
 
 

But it seems that I stumble on a software debug challenge after I replaced the dummy alternation in the regular expression above by the real list of 5142 function names which were found by the other discussed pattern.

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch --sp-file delete_unnecessary_checks2.cocci /usr/src/linux-stable/fs/btrfs/inode.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h Fatal error: exception Pcre.Error(_)

Regards,
Markus

I don't think it is a good approach to do anything related to 5142 different functions at once. It would be better to pick a small set of functions, and work on them carefully.

julia

A source code search with the following pattern found only six functions for further consideration.

@is_unnecessary_check@
expression data;
identifier work;
identifier release =~ "^kz?free$";
position pos;
type t;
@@
t work at pos(...)
{
...
( if (data) release(data);
| if (likely(data)) release(data);
)
...
}

I wonder a bit about this analysis result because the SmPL pattern I started this discussion thread with showed a few more update candidates.

I would prefer to handle a more complete fix pattern.

By the way: I read about corresponding software update approaches in an article "Best practices for a big patch series" by Wolfram Sang.

How are the chances to resolve the message "Fatal error: exception Pcre.Error(_)" with a proposed long alternation?

Regards,
Markus

Remember that ... matches the shortest path between what is before and what is after. So if there is another if test on the same data in the function, that will cause a failure. If yo udon't care to have this constraint, you can put when any on a ...

As you like, but I think that no Linux maintainer will accept a patch that makes more than a few changes, so doing them all at once is a waste of time. Anyway, you would have to split up the patches according to maintainer.

Pcre is a standard library. I know nothing about its implementation. But it is not surprising that a regular expression with over 5000 options exceeds its capacity.

julia

Are there any chances to find the source code place from which the exception "Pcre.Error(_)" was thrown?

Regards,
Markus

Set the OCAMLRUNPARAM environment variable to b in your shell. I don't know if it is necessary to recompile coccinelle. Then when it crashes you will get a backtrace.

julia

elfring at Sonne:~/Projekte/Coccinelle/janitor> export OCAMLRUNPARAM=b && spatch --sp-file delete_unnecessary_checks2.cocci /usr/src/linux-stable/fs/btrfs/inode.cinit_defs_builtins:
/usr/local/share/coccinelle/standard.h
Fatal error: exception Pcre.Error(_)
Called from file "engine.ml", line 206, characters 1-27
Called from file "engine.ml", line 359, characters 23-36

How much can this information help us here?

Regards,
Markus

Those two lines at least are not from Coccinelle, so I guess that they are from the PCRE implementation. But I don't think there is anything to do. The regular expression engine is just not likely to be designed to support such a large regular expression.

If you like you can give each of the function names one by one as a command line argument. Eg

spatch file.cocci file.c -D fn=whatever

and then in your semantic patch

@@
identifier virtual.fn;
@@

... use fn as an identifier ...

julia

Are there any chances to retrieve a more detailed error information?

I would try to compare the processing of big patterns with similar interfaces from other software.

I imagine that this approach will result also in different opinions about a useful patch granularity.
http://lwn.net/Articles/585782/

Regards,
Markus

In my opinion, the change you are proposing is controversial. Particular code instances may be corrected in a better way. If you try to do everything at once then the kernel maintainer will have a big block that he has to either accept or reject. If there is one change that is not made in the best way, then he will have to reject it.

On the other hand, if you do a few things at a time, then most of the individual things you do will perhaps be noncontroversial.

julia

Which SmPL "when variant" would you recommend here?

Regards,
Markus

when any

The following pattern variant does not show a different analysis result.

@is_unnecessary_check@
expression data;
identifier work;
identifier release =~ "^kz?free$";
position pos;
type t;
@@
t work at pos(...)
{
... when any
( if (data) release(data);
| if (likely(data)) release(data);
)
... when any
}

Regards,
Markus

A source code search with the following pattern found 893 functions which check their single parameter.

@safety_check@
identifier work, input;
type data_type;
position pos;
statement is, es;
@@
void work at pos(data_type input)
{
... when any
( if (input) is else es
| if (likely(input)) is else es
)
... when any
}

Such a name list could be integrated into the following pattern variant.

@Delete_unnecessary_checks@
expression x;
identifier release =~ "^(?x)
(?:
(?:kz?|slob_)free
|
(?:

      # alternation/disjunction of 893 strings ...
   )
)$";
@@
-if (x)
    release(x);

I get an interesting result for example. Do you find it useful?

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt --sp-file delete_unnecessary_checks3.cocci /usr/src/linux-stable/fs/btrfs/inode.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/fs/btrfs/inode.c

diff =
@@ -819,8 +819,7 @@ static u64 get_extent_allocation_hint(st
                        em = search_extent_mapping(em_tree, 0, 0);
                        if (em && em->block_start < EXTENT_MAP_LAST_BYTE)
                                alloc_hint = em->block_start;
-                       if (em)
-                               free_extent_map(em);
+                       free_extent_map(em);
                } else {
                        alloc_hint = em->block_start;
                        free_extent_map(em);

@@ -5137,8 +5136,7 @@ static int btrfs_dentry_delete(const str
 
 static void btrfs_dentry_release(struct dentry *dentry)
 {
-       if (dentry->d_fsdata)
-               kfree(dentry->d_fsdata);
+       kfree(dentry->d_fsdata);
 }
 
 static struct dentry *btrfs_lookup(struct inode *dir, struct dentry *dentry,

@@ -6362,8 +6360,7 @@ out:
 
        trace_btrfs_get_extent(root, em);
 
-       if (path)
-               btrfs_free_path(path);
+       btrfs_free_path(path);
        if (trans) {
                ret = btrfs_end_transaction(trans, root);
                if (!err)

Does the following pattern variant show also around 70 update candidates on your software development system?

@is_unnecessary_check@
expression data;
identifier work;
identifier release =~ "^(?x)
(?:
# Big regular expression for a metavariable constraint ...
)$";
position pos;
type t;
@@
t work at pos(...)
{
... when any
( if (data) release(data);
| if (likely(data)) release(data);
)
... when any
}

elfring at Sonne:~/Projekte/Coccinelle/janitor> date && spatch.opt --sp-file list_functions_with_unnecessary_checks1.cocci --dir /usr/src/linux-stable > list_functions_with_unnecessary_checks1.txt 2> list_functions_with_unnecessary_checks1-errors.txt && date
Mi 26. Feb 11:38:14 CET 2014
Mi 26. Feb 12:13:47 CET 2014

The log file contains messages like 'EXN:Invalid_argument("equal: abstract value")'. Do they need further considerations?

Is this intermediate source code analysis result good enough to expand the constructive discussion to a mailing list like "kernel janitors"?

Regards,
Markus

I checks on the three examples. They seem technically correct, in that the called function does immediately do a null test on the argument value. Personally, I still think that the null test at the call site conveys useful information, so I would rather have it there, but others may think differently.

On the other hand, you should not be using linux-stable when working on the Linux kernel. Patches should apply to linux-next, ie the very latest version of all of the code.

If you are convinced that dropping the null tests is a good idea, then you can submit the patch that makes the change to the relevant maintainers and mailing lists. Use scripts/get_maintainers.pl in the Linux kernel to find out who to sent it to. And study the documentation on submitting patches very carefully.

This is an unfortunate constraint on the use of PCRE regular expressions. I will try to track it down. Thanks for the report.

I don't think people on the mailing list want to discuss the issue in a general sense. They just want patches, and may comment on them.

julia

The discussed semantic patches can be tried out with every version a software developer or reviewer might be interested in. I am going to retry them with the newest source files as you suggest.

Thanks for your feedback.

- Do you need any excerpt from my log files?
- Does the message 'EXN:Invalid_argument("equal: abstractvalue")' indicate that the desired source code analysis is influenced in unwanted ways?

Regards,
Markus

No, the semantic patch should be enough to reproduce the problem. If I need more information, I will ask. The problem has no impact on the quality of the answers actually obtained.

julia

I have looked into my log files once more so that I can give you another feedback for a "regexp problem".

Now I make a different observation for the processing of SmPL constraints with a long function name list in the regular expression.

elfring at Sonne:~/Projekte/Coccinelle/janitor> OCAMLRUNPARAM=b spatch.opt --sp-file delete_unnecessary_checks3.cocci /usr/src/linux-stable/fs/proc/proc_sysctl.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/fs/proc/proc_sysctl.c

diff =
@@ -470,8 +470,7 @@ static struct dentry *proc_sys_lookup(st
        d_add(dentry, inode);
 
 out:
-       if (h)
-               sysctl_head_finish(h);
+       sysctl_head_finish(h);
        sysctl_head_finish(head);
        return err;
 }

elfring at Sonne:~/Projekte/Coccinelle/janitor> OCAMLRUNPARAM=b spatch.opt --sp-file list_functions_with_unnecessary_checks1.cocci /usr/src/linux-stable/fs/proc/proc_sysctl.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/fs/proc/proc_sysctl.c
Fatal error: exception Invalid_argument("equal: abstract value") Called from file "list.ml", line 86, characters 24-34
Called from file "list.ml", line 86, characters 24-34
Called from file "list.ml", line 86, characters 24-34
Called from file "list.ml", line 57, characters 20-23

Regards,
Markus

I can't reproduce this. I need the semantic patch.

julia

How do you think about to try out the scripts which I published yesterday evening?

Regards,
Markus

It seems that you could fix this issue with the software release "1.0.0-rc22".

I have tried out the discussed approach on the source files for the operating system "Linux 3.16.3" once more. Now I stumble on another error message.

elfring at Sonne:~/Projekte/Coccinelle/janitor> spatch.opt --version && OCAMLRUNPARAM=b spatch.opt --sp-file list_functions_with_unnecessary_checks1.cocci /usr/src/linux-stable/crypto/fcrypt.c spatch version 1.0.0-rc22 with Python support and with PCRE support
init_defs_builtins: /usr/local/share/coccinelle/standard.h
HANDLING: /usr/src/linux-stable/crypto/fcrypt.c
Fatal error: exception Failure("lexing: empty token")
Raised at file "lexing.ml", line 65, characters 15-37

Can this detail also be improved?

Regards,
Markus

Hello,

A couple of functions perform input parameter validation before their implementations will try further actions with side effects. Some calling functions perform similar safety checks.

Functions which release a system resource are often documented in the way that they tolerate the passing of a null pointer for example. I do not see a need because of this fact that a function caller repeats a corresponding check.

Now I would like to propose such a change again.

1. Extension of the infrastructure for the analysis tool "coccicheck" Semantic patch patterns can help to identify update candidates also in the
Linux source file hierarchy.

https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/scripts/coccinelle?id=79f0345fefaafb7cde301a830471edd21a37989b

2. Clarification for some automated update suggestions
My source code search approach found seventy functions at least which might
need another review and corresponding corrections for Linux 3.14-rc5. Further software development will point out even more potentially open issues.

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 18:15:34 +0100
Subject: [PATCH 1/5] Addition of a semantic patch file for showing unnecessary
checks before a few known functions

This semantic patch pattern tries to find source code places where a check is performed for an expression that is passed to a function (like "kfree") which checks this single parameter itself for usability. Redundant value or pointer checks can be avoided here.

The pattern contains a special comment in a regular expression for a SmPL constraint which supports extensions.

---
 .../deletions/delete_unnecessary_checks_template1.cocci     | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/delete_unnecessary_checks_template1.cocci

diff --git a/scripts/coccinelle/deletions/delete_unnecessary_checks_template1.cocci b/scripts/coccinelle/deletions/delete_unnecessary_checks_template1.cocci new file mode 100644
index 0000000..b092051

@@ -0,0 +1,13 @@
+ at Delete_unnecessary_checks@
+expression x;
+identifier release =~ "^(?x)
+(?:
+   (?:kz?|slob_)free
+|
+   (?:
+# Alternation placeholder
+   )
+)$";
+@@
+-if (x)
+    release(x);

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 18:38:43 +0100
Subject: [PATCH 2/5] Addition of a semantic patch file for listing of
functions that check their single parameter

This semantic patch pattern tries to find functions that check their single parameter for usability.

Example:
Null pointer checks are often performed as input parameter validation.

It uses Python statements to write information about the found source code places in a data format that is a variant of a CSV text file.

---
 .../list_input_parameter_validation1.cocci         | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/list_input_parameter_validation1.cocci

b/scripts/coccinelle/deletions/list_input_parameter_validation1.cocci new file mode 100644
index 0000000..b0a5a52

'"source file"', "line", "column")))
+ print("\r\n".join(result))
+else:
+ sys.stderr.write("No result for this analysis!\n")

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 18:58:30 +0100
Subject: [PATCH 3/5] Addition of a SQLite script for a text file import

A script was added so that a text file which was previously generated can be imported into a SQLite data base table.
http://sqlite.org/sqlite.html

The shown file name can be adjusted by a make file for example.

---
 .../coccinelle/deletions/handle_function_list_template.sqlite    | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/handle_function_list_template.sqlite

b/scripts/coccinelle/deletions/handle_function_list_template.sqlite new file mode 100644
index 0000000..bec366c

@@ -0,0 +1,9 @@
+create table positions(function text,
+                       data_type text,
+                       parameter text,
+                       source_file text,
+                       line integer,
+                       column integer);
+.separator "|"
+.import list_input_pointer_validation1.txt positions
+.header OFF

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 19:10:32 +0100
Subject: [PATCH 4/5] Addition of a semantic patch file for listing of
unnecessary checks before a few known functions

This semantic patch pattern tries to find source code places where a check is performed for an expression that is passed to a function (like "kfree") which checks this single parameter itself for usability. Redundant value or pointer checks can be avoided here.

The pattern contains a special comment in a regular expression for a SmPL constraint which supports extensions.

It uses Python statements to write information about the found places in a data format that is a variant of a CSV text file.

---
 ...nctions_with_unnecessary_checks_template1.cocci | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci

diff --git a/scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci b/scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci new file mode 100644
index 0000000..d2637e8
--- /dev/null
+++
b/scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci @@ -0,0 +1,59 @@
+ at initialize:python@
+@@
+import sys
+result = []
+mark = ['"', '', '"']
+delimiter = '|'
+
+def store_positions(fun, point, places):
+ """Add source code positions to an internal list."""
+ for place in places:
+ fields = []
+ fields.append(fun)
+
+ fields.append(point)
+
+ mark[1] = place.file.replace('"', '""')
+ fields.append(''.join(mark))
+

+        fields.append(place.line)
+        fields.append(str(int(place.column) + 1))
+        result.append(delimiter.join(fields))

+
+ at is_unnecessary_check@
+expression data;
+identifier work;
+identifier release =~ "^(?x)
+(?:
+ (?:kz?|slob_)free
+|
+ (?:
+# Alternation placeholder
+ )
+)$";
+position pos;
+type t;
+@@
+ t work at pos(...)
+ {
+ ... when any
+( if (data) release(data);
+| if (likely(data)) release(data);
+)
+ ... when any
+ }
+
+ at script:python collection depends on is_unnecessary_check@ +fun << is_unnecessary_check.work;
+point << is_unnecessary_check.data;
+places << is_unnecessary_check.pos;
+@@
+store_positions(fun, point, places)
+
+@finalize:python@
+@@
+if result:
+ result.insert(0, delimiter.join(("function", '"parameter"', '"source file"',
"line", "column")))
+ print("\r\n".join(result))
+else:
+ sys.stderr.write("No result for this analysis!\n")

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 19:40:43 +0100
Subject: [PATCH 5/5] Addition of a make file for build automation

This script can be used to combine some input files for the desired data output which will eventually show update candidates for further source code review. Some build targets were defined. Values for the used make variables can be adjusted by parameters on the command line as usual.

A few implementation details might need more fine-tuning.
- Automatic determination of the Linux source directory from a calling make process

- Setting of an extra output directory for the generated files

---
 scripts/coccinelle/deletions/makefile | 126 ++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)
 create mode 100644 scripts/coccinelle/deletions/makefile

b/scripts/coccinelle/deletions/makefile
new file mode 100644
index 0000000..6464bae

@@ -0,0 +1,126 @@
+SED=sed
+SPATCH=spatch.opt --sp-file
+RM=rm -f
+LINUX_SOURCE_DIR=/usr/src/linux-stable
+EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER=list_input_parameter_validation1.cocci
+SQLITE=sqlite3
+SQLITE_IMPORT_SCRIPT=handle_function_list.sqlite
+SQLITE_IMPORT_SCRIPT_TEMPLATE=handle_function_list_template.sqlite
+RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER=list_input_parameter_validation1.txt
+RESULT_SQL_DATA_BASE=result.db
+RESULT_FUNCTIONS_WITH_PREFIX=add_prefix-SQL.txt
+RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_LIST=list_functions_with_unnecessary_checks1.txt
+RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_PATCH=functions_with_unnecessary_checks1.diff
+LOG_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER=list_input_parameter_validation1-errors.txt
+LOG_LIST_FUNCTIONS_WITH_UNNECESSARY_CHECKS=list_functions_with_unnecessary_checks1-errors.txt
+LOG_PATCH_FUNCTIONS_WITH_UNNECESSARY_CHECKS=functions_with_unnecessary_checks1-errors.txt
+LIST_PATTERN_TEMPLATE=list_functions_with_unnecessary_checks_template1.cocci
+LIST_PATTERN=list_functions_with_unnecessary_checks1.cocci
+PATCH_PATTERN_TEMPLATE=delete_unnecessary_checks_template1.cocci
+PATCH_PATTERN=delete_unnecessary_checks1.cocci
+ESCAPING=XY=$$(< $(RESULT_FUNCTIONS_WITH_PREFIX)) \
+         && XY=$${XY/|/ } \
+         && XY=$${XY//%/\\%} \
+         && $(SED) "s%\# Alternation placeholder%$${XY//$$'\n'/$$'\\\\\n'}%"
+TEXT1=A pattern file was built from which a
+TEXT2=was generated. Good luck with source code review!
+
+default: build_update_candidate_list
+
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER): \
+$(EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+	$(SPATCH) $(EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+-dir $(LINUX_SOURCE_DIR) \
+> $@ 2> $(LOG_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+
+# This replacement action is needed for the use case that the corresponding
+# variable was overridden.
+$(SQLITE_IMPORT_SCRIPT): $(SQLITE_IMPORT_SCRIPT_TEMPLATE)
+	$(SED) "s%import list_input_pointer_validation1\.txt%import $(subst

%,\%,$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER))%" \
+$(SQLITE_IMPORT_SCRIPT_TEMPLATE) > $@
+
+$(RESULT_SQL_DATA_BASE) $(RESULT_FUNCTIONS_WITH_PREFIX): \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+$(SQLITE_IMPORT_SCRIPT)
+ @$(RM) $(RESULT_SQL_DATA_BASE)
+ $(SQLITE) -init $(SQLITE_IMPORT_SCRIPT) $(RESULT_SQL_DATA_BASE) \
+'select '\'' | '\'' || function from positions group by function order by
function desc;' \
+> $(RESULT_FUNCTIONS_WITH_PREFIX)
+
+$(LIST_PATTERN): $(RESULT_FUNCTIONS_WITH_PREFIX)
+ $(ESCAPING) $(LIST_PATTERN_TEMPLATE) > $@
+
+$(PATCH_PATTERN): $(RESULT_FUNCTIONS_WITH_PREFIX)
+ $(ESCAPING) $(PATCH_PATTERN_TEMPLATE) > $@
+
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_LIST): $(LIST_PATTERN)
+ $(SPATCH) $(LIST_PATTERN) -dir $(LINUX_SOURCE_DIR) \
+> $@ 2> $(LOG_LIST_FUNCTIONS_WITH_UNNECESSARY_CHECKS)
+
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_PATCH): $(PATCH_PATTERN)
+ $(SPATCH) $(PATCH_PATTERN) -dir $(LINUX_SOURCE_DIR) \
+> $@ 2> $(LOG_PATCH_FUNCTIONS_WITH_UNNECESSARY_CHECKS)
+
+build_check_list generate_list_of_functions_which_check_their_single_parameter: \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+ @echo 'The list of functions which check their single parameter was generated.'
+
+build_import_script: $(SQLITE_IMPORT_SCRIPT)
+ @echo 'A script was generated which should contain appropriate parameters for
a data base.'
+
+build_data_base: $(RESULT_SQL_DATA_BASE)
+ @echo 'A SQL data base was built.'
+
+build_alternation add_prefix_to_functions: build_data_base
+ @echo 'The function name list was converted to a component for a regular
expression.'
+
+build_list_pattern: $(LIST_PATTERN)
+ @echo '$(TEXT1) list can be generated.'
+
+build_patch_pattern: $(PATCH_PATTERN)
+ @echo '$(TEXT1) patch can be generated.'
+
+build_update_candidate_list show_list_of_update_candidates: build_list_pattern \
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_LIST)
+ @echo 'The list of update candidates $(TEXT2)'
+
+build_patch show_update_suggestion: build_patch_pattern \
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_PATCH)
+ @echo 'A patch file $(TEXT2)'
+
+all: build_update_candidate_list build_patch
+
+clean:
+ $(RM) *-errors.txt \
+$(LIST_PATTERN) \
+$(PATCH_PATTERN) \
+$(RESULT_FUNCTIONS_WITH_PREFIX) \
+$(RESULT_SQL_DATA_BASE) \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+$(SQLITE_IMPORT_SCRIPT)
+
+delete_data_base:
+ $(RM) $(RESULT_SQL_DATA_BASE) \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+
+.PHONY: all \
+build_check_list \
+build_data_base \
+build_import_script \
+build_list_pattern \
+build_patch \
+build_patch_pattern \
+build_update_candidate_list \
+clean \
+default \
+delete_data_base \
+generate_list_of_functions_which_check_their_single_parameter \
+show_list_of_update_candidates \
+show_update_suggestion
+
+
+# The following input files should not need further actions here. +$(EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+$(SQLITE_IMPORT_SCRIPT_TEMPLATE) \
+$(LIST_PATTERN_TEMPLATE) \
+$(PATCH_PATTERN_TEMPLATE): ;

1.9.0

Hello,

A couple of functions perform input parameter validation before their implementations will try further actions with side effects. Some calling functions perform similar safety checks.

Functions which release a system resource are occasionally documented in the way that they tolerate the passing of a null pointer for example.
I do not see a need because of this fact that a function caller repeats a corresponding check.

Now I would like to propose such a change again.

1. Extension of the infrastructure for the analysis tool "coccicheck" Semantic patch patterns can help to identify update candidates also in the
Linux source file hierarchy.

https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/scripts/coccinelle?id=79f0345fefaafb7cde301a830471edd21a37989b

Would you like to reconsider an approach which was discussed with a subject
like "scripts/coccinelle/free: Delete NULL test before freeing functions?" a while ago?
https://lkml.org/lkml/2014/8/9/36
https://groups.google.com/d/msg/linux.kernel/rIWfYsRRW6I/cTs6y0STf2cJ

2. Clarification for some automated update suggestions
My source code search approach found 227 functions with the help of the
software "Coccinelle 1.0.0-rc22" at least which might need another review and corresponding corrections for Linux 3.16.3. Further software development will point out even more potentially open issues.

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 18:15:34 +0100
Subject: [PATCH 1/5] Addition of a semantic patch file for showing unnecessary
checks before a few known functions

This semantic patch pattern tries to find source code places where a check is performed for an expression that is passed to a function (like "kfree") which checks this single parameter itself for usability. Redundant value or pointer checks can be avoided here.

The pattern contains a special comment in a regular expression for a SmPL constraint which supports extensions.

---
 .../deletions/delete_unnecessary_checks_template1.cocci     | 13 +++++++++++++
 1 file changed, 13 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/delete_unnecessary_checks_template1.cocci

diff --git a/scripts/coccinelle/deletions/delete_unnecessary_checks_template1.cocci b/scripts/coccinelle/deletions/delete_unnecessary_checks_template1.cocci new file mode 100644
index 0000000..b092051

@@ -0,0 +1,13 @@
+ at Delete_unnecessary_checks@
+expression x;
+identifier release =~ "^(?x)
+(?:
+   (?:kz?|slob_)free
+|
+   (?:
+# Alternation placeholder
+   )
+)$";
+@@
+-if (x)
+    release(x);

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 18:38:43 +0100
Subject: [PATCH 2/5] Addition of a semantic patch file for listing of
functions that check their single parameter

This semantic patch pattern tries to find functions that check their single parameter for usability.

Example:
Null pointer checks are often performed as input parameter validation.

It uses Python statements to write information about the found source code places in a data format that is a variant of a CSV text file.

---
 .../list_input_parameter_validation1.cocci         | 55 ++++++++++++++++++++++
 1 file changed, 55 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/list_input_parameter_validation1.cocci

b/scripts/coccinelle/deletions/list_input_parameter_validation1.cocci new file mode 100644
index 0000000..b0a5a52

'"source file"', "line", "column")))
+ print("\r\n".join(result))
+else:
+ sys.stderr.write("No result for this analysis!\n")

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 18:58:30 +0100
Subject: [PATCH 3/5] Addition of a SQLite script for a text file import

A script was added so that a text file which was previously generated can be imported into a SQLite data base table.
http://sqlite.org/sqlite.html

The shown file name can be adjusted by a make file for example.

---
 .../coccinelle/deletions/handle_function_list_template.sqlite    | 9 +++++++++
 1 file changed, 9 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/handle_function_list_template.sqlite

b/scripts/coccinelle/deletions/handle_function_list_template.sqlite new file mode 100644
index 0000000..bec366c

@@ -0,0 +1,9 @@
+create table positions(function text,
+                       data_type text,
+                       parameter text,
+                       source_file text,
+                       line integer,
+                       column integer);
+.separator "|"
+.import list_input_pointer_validation1.txt positions
+.header OFF

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 19:10:32 +0100
Subject: [PATCH 4/5] Addition of a semantic patch file for listing of
unnecessary checks before a few known functions

This semantic patch pattern tries to find source code places where a check is performed for an expression that is passed to a function (like "kfree") which checks this single parameter itself for usability. Redundant value or pointer checks can be avoided here.

The pattern contains a special comment in a regular expression for a SmPL constraint which supports extensions.

It uses Python statements to write information about the found places in a data format that is a variant of a CSV text file.

---
 ...nctions_with_unnecessary_checks_template1.cocci | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)
 create mode 100644

scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci

diff --git a/scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci b/scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci new file mode 100644
index 0000000..d2637e8
--- /dev/null
+++
b/scripts/coccinelle/deletions/list_functions_with_unnecessary_checks_template1.cocci @@ -0,0 +1,59 @@
+ at initialize:python@
+@@
+import sys
+result = []
+mark = ['"', '', '"']
+delimiter = '|'
+
+def store_positions(fun, point, places):
+ """Add source code positions to an internal list."""
+ for place in places:
+ fields = []
+ fields.append(fun)
+
+ fields.append(point)
+
+ mark[1] = place.file.replace('"', '""')
+ fields.append(''.join(mark))
+

+        fields.append(place.line)
+        fields.append(str(int(place.column) + 1))
+        result.append(delimiter.join(fields))

+
+ at is_unnecessary_check@
+expression data;
+identifier work;
+identifier release =~ "^(?x)
+(?:
+ (?:kz?|slob_)free
+|
+ (?:
+# Alternation placeholder
+ )
+)$";
+position pos;
+type t;
+@@
+ t work at pos(...)
+ {
+ ... when any
+( if (data) release(data);
+| if (likely(data)) release(data);
+)
+ ... when any
+ }
+
+ at script:python collection depends on is_unnecessary_check@ +fun << is_unnecessary_check.work;
+point << is_unnecessary_check.data;
+places << is_unnecessary_check.pos;
+@@
+store_positions(fun, point, places)
+
+@finalize:python@
+@@
+if result:
+ result.insert(0, delimiter.join(("function", '"parameter"', '"source file"',
"line", "column")))
+ print("\r\n".join(result))
+else:
+ sys.stderr.write("No result for this analysis!\n")

1.9.0

From: Markus Elfring <[email protected]>
Date: Wed, 5 Mar 2014 19:40:43 +0100
Subject: [PATCH 5/5] Addition of a make file for build automation

This script can be used to combine some input files for the desired data output which will eventually show update candidates for further source code review. Some build targets were defined. Values for the used make variables can be adjusted by parameters on the command line as usual.

A few implementation details might need more fine-tuning.
- Automatic determination of the Linux source directory from a calling make process

- Setting of an extra output directory for the generated files

---
 scripts/coccinelle/deletions/makefile | 126 ++++++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)
 create mode 100644 scripts/coccinelle/deletions/makefile

b/scripts/coccinelle/deletions/makefile
new file mode 100644
index 0000000..6464bae

@@ -0,0 +1,126 @@
+SED=sed
+SPATCH=spatch.opt --sp-file
+RM=rm -f
+LINUX_SOURCE_DIR=/usr/src/linux-stable
+EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER=list_input_parameter_validation1.cocci
+SQLITE=sqlite3
+SQLITE_IMPORT_SCRIPT=handle_function_list.sqlite
+SQLITE_IMPORT_SCRIPT_TEMPLATE=handle_function_list_template.sqlite
+RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER=list_input_parameter_validation1.txt
+RESULT_SQL_DATA_BASE=result.db
+RESULT_FUNCTIONS_WITH_PREFIX=add_prefix-SQL.txt
+RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_LIST=list_functions_with_unnecessary_checks1.txt
+RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_PATCH=functions_with_unnecessary_checks1.diff
+LOG_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER=list_input_parameter_validation1-errors.txt
+LOG_LIST_FUNCTIONS_WITH_UNNECESSARY_CHECKS=list_functions_with_unnecessary_checks1-errors.txt
+LOG_PATCH_FUNCTIONS_WITH_UNNECESSARY_CHECKS=functions_with_unnecessary_checks1-errors.txt
+LIST_PATTERN_TEMPLATE=list_functions_with_unnecessary_checks_template1.cocci
+LIST_PATTERN=list_functions_with_unnecessary_checks1.cocci
+PATCH_PATTERN_TEMPLATE=delete_unnecessary_checks_template1.cocci
+PATCH_PATTERN=delete_unnecessary_checks1.cocci
+ESCAPING=XY=$$(< $(RESULT_FUNCTIONS_WITH_PREFIX)) \
+         && XY=$${XY/|/ } \
+         && XY=$${XY//%/\\%} \
+         && $(SED) "s%\# Alternation placeholder%$${XY//$$'\n'/$$'\\\\\n'}%"
+TEXT1=A pattern file was built from which a
+TEXT2=was generated. Good luck with source code review!
+
+default: build_update_candidate_list
+
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER): \
+$(EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+	$(SPATCH) $(EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+-dir $(LINUX_SOURCE_DIR) \
+> $@ 2> $(LOG_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+
+# This replacement action is needed for the use case that the corresponding
+# variable was overridden.
+$(SQLITE_IMPORT_SCRIPT): $(SQLITE_IMPORT_SCRIPT_TEMPLATE)
+	$(SED) "s%import list_input_pointer_validation1\.txt%import $(subst

%,\%,$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER))%" \
+$(SQLITE_IMPORT_SCRIPT_TEMPLATE) > $@
+
+$(RESULT_SQL_DATA_BASE) $(RESULT_FUNCTIONS_WITH_PREFIX): \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+$(SQLITE_IMPORT_SCRIPT)
+ @$(RM) $(RESULT_SQL_DATA_BASE)
+ $(SQLITE) -init $(SQLITE_IMPORT_SCRIPT) $(RESULT_SQL_DATA_BASE) \
+'select '\'' | '\'' || function from positions group by function order by
function desc;' \
+> $(RESULT_FUNCTIONS_WITH_PREFIX)
+
+$(LIST_PATTERN): $(RESULT_FUNCTIONS_WITH_PREFIX)
+ $(ESCAPING) $(LIST_PATTERN_TEMPLATE) > $@
+
+$(PATCH_PATTERN): $(RESULT_FUNCTIONS_WITH_PREFIX)
+ $(ESCAPING) $(PATCH_PATTERN_TEMPLATE) > $@
+
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_LIST): $(LIST_PATTERN)
+ $(SPATCH) $(LIST_PATTERN) -dir $(LINUX_SOURCE_DIR) \
+> $@ 2> $(LOG_LIST_FUNCTIONS_WITH_UNNECESSARY_CHECKS)
+
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_PATCH): $(PATCH_PATTERN)
+ $(SPATCH) $(PATCH_PATTERN) -dir $(LINUX_SOURCE_DIR) \
+> $@ 2> $(LOG_PATCH_FUNCTIONS_WITH_UNNECESSARY_CHECKS)
+
+build_check_list generate_list_of_functions_which_check_their_single_parameter: \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+ @echo 'The list of functions which check their single parameter was generated.'
+
+build_import_script: $(SQLITE_IMPORT_SCRIPT)
+ @echo 'A script was generated which should contain appropriate parameters for
a data base.'
+
+build_data_base: $(RESULT_SQL_DATA_BASE)
+ @echo 'A SQL data base was built.'
+
+build_alternation add_prefix_to_functions: build_data_base
+ @echo 'The function name list was converted to a component for a regular
expression.'
+
+build_list_pattern: $(LIST_PATTERN)
+ @echo '$(TEXT1) list can be generated.'
+
+build_patch_pattern: $(PATCH_PATTERN)
+ @echo '$(TEXT1) patch can be generated.'
+
+build_update_candidate_list show_list_of_update_candidates: build_list_pattern \
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_LIST)
+ @echo 'The list of update candidates $(TEXT2)'
+
+build_patch show_update_suggestion: build_patch_pattern \
+$(RESULT_FUNCTIONS_WITH_UNNECESSARY_CHECKS_AS_PATCH)
+ @echo 'A patch file $(TEXT2)'
+
+all: build_update_candidate_list build_patch
+
+clean:
+ $(RM) *-errors.txt \
+$(LIST_PATTERN) \
+$(PATCH_PATTERN) \
+$(RESULT_FUNCTIONS_WITH_PREFIX) \
+$(RESULT_SQL_DATA_BASE) \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+$(SQLITE_IMPORT_SCRIPT)
+
+delete_data_base:
+ $(RM) $(RESULT_SQL_DATA_BASE) \
+$(RESULT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER)
+
+.PHONY: all \
+build_check_list \
+build_data_base \
+build_import_script \
+build_list_pattern \
+build_patch \
+build_patch_pattern \
+build_update_candidate_list \
+clean \
+default \
+delete_data_base \
+generate_list_of_functions_which_check_their_single_parameter \
+show_list_of_update_candidates \
+show_update_suggestion
+
+
+# The following input files should not need further actions here. +$(EXTRACT_FUNCTIONS_THAT_CHECK_THEIR_SINGLE_PARAMETER) \
+$(SQLITE_IMPORT_SCRIPT_TEMPLATE) \
+$(LIST_PATTERN_TEMPLATE) \
+$(PATCH_PATTERN_TEMPLATE): ;

1.9.0

Would you like to integrate the following proposal into your source code repository?

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Wed, 22 Oct 2014 15:45:22 +0200
Subject: [PATCH] GPU-DRM-nouveau: Deletion of unnecessary checks before two
function calls

A semantic patch approach was proposed with the subject "[PATCH with Coccinelle?] Deletion of unnecessary checks before specific function calls" on 2014-03-05.
https://lkml.org/lkml/2014/3/5/344
http://article.gmane.org/gmane.comp.version-control.coccinelle/3513/

This patch pattern application was repeated with the help of the software "Coccinelle 1.0.0-rc22" on the source files for Linux 3.17.1. An extract of the automatically generated update suggestions is shown here.

It was determined that the affected source code places call functions which perform input parameter validation already. It is therefore not needed that a similar safety check is repeated at the call site.

---
 drivers/gpu/drm/nouveau/core/core/handle.c | 3 +--
 drivers/gpu/drm/nouveau/nouveau_drm.c      | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

b/drivers/gpu/drm/nouveau/core/core/handle.c
index a490b80..75d0c2c 100644

@@ -219,8 +219,7 @@ nouveau_handle_get_cinst(struct nouveau_object *engctx, u32

cinst)
void
nouveau_handle_put(struct nouveau_handle *handle) {
- if (handle)
- nouveau_namedb_put(handle);
+ nouveau_namedb_put(handle);
}

int

b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 5723807..5c29079 100644

@@ -512,8 +512,7 @@ nouveau_drm_unload(struct drm_device *dev)
 	nouveau_vga_fini(drm);
 
 	nvif_device_fini(&drm->device);
-	if (drm->hdmi_device)
-		pci_dev_put(drm->hdmi_device);
+	pci_dev_put(drm->hdmi_device);
 	nouveau_cli_destroy(&drm->client);
 	return 0;
 }

2.1.2

From: Markus Elfring <[email protected]>
Date: Sun, 5 Jul 2015 20:15:12 +0200

The functions nvkm_namedb_put() and pci_dev_put() test whether their argument is NULL and then return immediately.
Thus the test around the calls is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/gpu/drm/nouveau/nouveau_drm.c      | 3 +--
 drivers/gpu/drm/nouveau/nvkm/core/handle.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

index 36b40c9..8ca3d0c 100644
@@ -515,8 +515,7 @@ nouveau_drm_unload(struct drm_device *dev)
 	nouveau_vga_fini(drm);
 
 	nvif_device_fini(&drm->device);
-	if (drm->hdmi_device)
-		pci_dev_put(drm->hdmi_device);
+	pci_dev_put(drm->hdmi_device);
 	nouveau_cli_destroy(&drm->client);
 	return 0;
 }

index dc7ff10..165f3ff 100644
@@ -216,6 +216,5 @@ nvkm_handle_get_cinst(struct nvkm_object *engctx, u32 cinst)
 void
 nvkm_handle_put(struct nvkm_handle *handle)
 {
-	if (handle)
-		nvkm_namedb_put(handle);
+	nvkm_namedb_put(handle);
 }

2.4.5

From: Markus Elfring <[email protected]>
Date: Wed, 20 Jul 2016 19:43:27 +0200

The pci_dev_put() function tests whether its argument is NULL and then returns immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 66c1280..7b09841 100644
@@ -525,8 +525,7 @@ nouveau_drm_unload(struct drm_device *dev)
 	nouveau_vga_fini(drm);
 
 	nvif_device_fini(&drm->device);
-	if (drm->hdmi_device)
-		pci_dev_put(drm->hdmi_device);
+	pci_dev_put(drm->hdmi_device);
 	nouveau_cli_destroy(&drm->client);
 	return 0;
 }

2.9.2

Would you like to integrate the following proposal into your source code repository?

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Wed, 22 Oct 2014 18:28:12 +0200
Subject: [PATCH] GPU-DRM-GMA500: Deletion of unnecessary checks before two
function calls

A semantic patch approach was proposed with the subject "[PATCH with Coccinelle?] Deletion of unnecessary checks before specific function calls" on 2014-03-05.
https://lkml.org/lkml/2014/3/5/344
http://article.gmane.org/gmane.comp.version-control.coccinelle/3513/

This patch pattern application was repeated with the help of the software "Coccinelle 1.0.0-rc22" on the source files for Linux 3.17.1. An extract of the automatically generated update suggestions is shown here.

It was determined that the affected source code places call functions which perform input parameter validation already. It is therefore not needed that a similar safety check is repeated at the call site.

---
 drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 3 +--
 drivers/gpu/drm/gma500/cdv_intel_lvds.c | 9 +++------
 drivers/gpu/drm/gma500/oaktrail_lvds.c  | 3 +--
 drivers/gpu/drm/gma500/psb_drv.c        | 3 +--
 drivers/gpu/drm/gma500/psb_intel_lvds.c | 9 +++------
 5 files changed, 9 insertions(+), 18 deletions(-)

b/drivers/gpu/drm/gma500/cdv_intel_hdmi.c
index 4268bf2..0d69624 100644

@@ -246,8 +246,7 @@ static void cdv_hdmi_destroy(struct drm_connector *connector)
 {
 	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
 
-	if (gma_encoder->i2c_bus)
-		psb_intel_i2c_destroy(gma_encoder->i2c_bus);
+	psb_intel_i2c_destroy(gma_encoder->i2c_bus);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);

b/drivers/gpu/drm/gma500/cdv_intel_lvds.c
index 0b77039..8f24013 100644

@@ -444,8 +444,7 @@ static void cdv_intel_lvds_destroy(struct drm_connector

*connector)
{
struct gma_encoder *gma_encoder = gma_attached_encoder(connector);

- if (gma_encoder->i2c_bus)
- psb_intel_i2c_destroy(gma_encoder->i2c_bus);
+ psb_intel_i2c_destroy(gma_encoder->i2c_bus);
drm_connector_unregister(connector);
drm_connector_cleanup(connector);
kfree(connector);
@@ -780,12 +779,10 @@ out:
failed_find:
mutex_unlock(&dev->mode_config.mutex);
printk(KERN_ERR "Failed find\n");
- if (gma_encoder->ddc_bus)
- psb_intel_i2c_destroy(gma_encoder->ddc_bus);
+ psb_intel_i2c_destroy(gma_encoder->ddc_bus);
failed_ddc:
printk(KERN_ERR "Failed DDC\n");
- if (gma_encoder->i2c_bus)
- psb_intel_i2c_destroy(gma_encoder->i2c_bus);
+ psb_intel_i2c_destroy(gma_encoder->i2c_bus);
failed_blc_i2c:
printk(KERN_ERR "Failed BLC\n");
drm_encoder_cleanup(encoder);

b/drivers/gpu/drm/gma500/oaktrail_lvds.c
index 0d39da6..49c5c415 100644

@@ -411,8 +411,7 @@ failed_find:
 	mutex_unlock(&dev->mode_config.mutex);
 
 	dev_dbg(dev->dev, "No LVDS modes found, disabling.\n");
-	if (gma_encoder->ddc_bus)
-		psb_intel_i2c_destroy(gma_encoder->ddc_bus);
+	psb_intel_i2c_destroy(gma_encoder->ddc_bus);
 
 /* failed_ddc: */
 

index 6ec3a90..0efe165 100644
@@ -210,8 +210,7 @@ static int psb_driver_unload(struct drm_device *dev)
 			iounmap(dev_priv->aux_reg);
 			dev_priv->aux_reg = NULL;
 		}
-		if (dev_priv->aux_pdev)
-			pci_dev_put(dev_priv->aux_pdev);
+		pci_dev_put(dev_priv->aux_pdev);
 
 		/* Destroy VBT data */
 		psb_intel_destroy_bios(dev);

b/drivers/gpu/drm/gma500/psb_intel_lvds.c
index 88aad95..e73c3f9 100644

@@ -561,8 +561,7 @@ void psb_intel_lvds_destroy(struct drm_connector *connector)
 	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
 	struct psb_intel_lvds_priv *lvds_priv = gma_encoder->dev_priv;
 
-	if (lvds_priv->ddc_bus)
-		psb_intel_i2c_destroy(lvds_priv->ddc_bus);
+	psb_intel_i2c_destroy(lvds_priv->ddc_bus);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);

@@ -834,11 +833,9 @@ out:
 
 failed_find:
 	mutex_unlock(&dev->mode_config.mutex);
-	if (lvds_priv->ddc_bus)
-		psb_intel_i2c_destroy(lvds_priv->ddc_bus);
+	psb_intel_i2c_destroy(lvds_priv->ddc_bus);
 failed_ddc:
-	if (lvds_priv->i2c_bus)
-		psb_intel_i2c_destroy(lvds_priv->i2c_bus);
+	psb_intel_i2c_destroy(lvds_priv->i2c_bus);
 failed_blc_i2c:
 	drm_encoder_cleanup(encoder);
 	drm_connector_cleanup(connector);

2.1.2

What platforms have you tested the code on at this point ?

Alan

None. - My "test computer" does not provide the corresponding hardware for the affected driver source files.

Regards,
Markus

I still have the hardware in use and I am willing to test.

If necessary, please send me a URL to the snapshot of the kernel with the modified code. I can compile this (takes about 8 hours ;-) on the high speed atom n2600) and run a couple of random tests, such as starting the WM, browsing some webpages, changing resolution, and run a 2D benchmark.

Greetings,
Arthur Borsboom

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20141026/a28726a1/attachment-0001.html>

From: Markus Elfring <[email protected]>
Date: Fri, 22 Jul 2016 10:30:30 +0200

The functions pci_dev_put() and psb_intel_i2c_destroy() test whether their argument is NULL and then return immediately.
Thus the tests around their calls are not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/gpu/drm/gma500/cdv_intel_hdmi.c | 3 +--
 drivers/gpu/drm/gma500/cdv_intel_lvds.c | 9 +++------
 drivers/gpu/drm/gma500/psb_drv.c        | 6 ++----
 drivers/gpu/drm/gma500/psb_intel_lvds.c | 9 +++------
 4 files changed, 9 insertions(+), 18 deletions(-)

index 28f9d90..563f193 100644
@@ -246,8 +246,7 @@ static void cdv_hdmi_destroy(struct drm_connector *connector)
 {
 	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
 
-	if (gma_encoder->i2c_bus)
-		psb_intel_i2c_destroy(gma_encoder->i2c_bus);
+	psb_intel_i2c_destroy(gma_encoder->i2c_bus);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);

index 813ef23..38dc890 100644
@@ -444,8 +444,7 @@ static void cdv_intel_lvds_destroy(struct drm_connector *connector)
 {
 	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
 
-	if (gma_encoder->i2c_bus)
-		psb_intel_i2c_destroy(gma_encoder->i2c_bus);
+	psb_intel_i2c_destroy(gma_encoder->i2c_bus);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);

@@ -780,12 +779,10 @@ out:
 failed_find:
 	mutex_unlock(&dev->mode_config.mutex);
 	printk(KERN_ERR "Failed find\n");
-	if (gma_encoder->ddc_bus)
-		psb_intel_i2c_destroy(gma_encoder->ddc_bus);
+	psb_intel_i2c_destroy(gma_encoder->ddc_bus);
 failed_ddc:
 	printk(KERN_ERR "Failed DDC\n");
-	if (gma_encoder->i2c_bus)
-		psb_intel_i2c_destroy(gma_encoder->i2c_bus);
+	psb_intel_i2c_destroy(gma_encoder->i2c_bus);
 failed_blc_i2c:
 	printk(KERN_ERR "Failed BLC\n");
 	drm_encoder_cleanup(encoder);

index 82b8ce4..50eb944f 100644
@@ -210,10 +210,8 @@ static int psb_driver_unload(struct drm_device *dev)
 			iounmap(dev_priv->aux_reg);
 			dev_priv->aux_reg = NULL;
 		}
-		if (dev_priv->aux_pdev)
-			pci_dev_put(dev_priv->aux_pdev);
-		if (dev_priv->lpc_pdev)
-			pci_dev_put(dev_priv->lpc_pdev);
+		pci_dev_put(dev_priv->aux_pdev);
+		pci_dev_put(dev_priv->lpc_pdev);
 
 		/* Destroy VBT data */
 		psb_intel_destroy_bios(dev);

index b1b9331..e55733c 100644
@@ -561,8 +561,7 @@ void psb_intel_lvds_destroy(struct drm_connector *connector)
 	struct gma_encoder *gma_encoder = gma_attached_encoder(connector);
 	struct psb_intel_lvds_priv *lvds_priv = gma_encoder->dev_priv;
 
-	if (lvds_priv->ddc_bus)
-		psb_intel_i2c_destroy(lvds_priv->ddc_bus);
+	psb_intel_i2c_destroy(lvds_priv->ddc_bus);
 	drm_connector_unregister(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);

@@ -835,11 +834,9 @@ out:
 
 failed_find:
 	mutex_unlock(&dev->mode_config.mutex);
-	if (lvds_priv->ddc_bus)
-		psb_intel_i2c_destroy(lvds_priv->ddc_bus);
+	psb_intel_i2c_destroy(lvds_priv->ddc_bus);
 failed_ddc:
-	if (lvds_priv->i2c_bus)
-		psb_intel_i2c_destroy(lvds_priv->i2c_bus);
+	psb_intel_i2c_destroy(lvds_priv->i2c_bus);
 failed_blc_i2c:
 	drm_encoder_cleanup(encoder);
 	drm_connector_cleanup(connector);

2.9.2

Two more applied. And your threading still looks funny, all the patches are in-reply to one I don't even have.
-Daniel

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

From: Markus Elfring <[email protected]>
Date: Wed, 22 Oct 2014 19:39:21 +0200
Subject: [PATCH] IOMMU-MSM: Deletion of unnecessary checks before the function
call "clk_disable"

A semantic patch approach was proposed with the subject "[PATCH with Coccinelle?] Deletion of unnecessary checks before specific function calls" on 2014-03-05.
https://lkml.org/lkml/2014/3/5/344
http://article.gmane.org/gmane.comp.version-control.coccinelle/3513/

This patch pattern application was repeated with the help of the software "Coccinelle 1.0.0-rc22" on the source files for Linux 3.17.1. An extract of the automatically generated update suggestions is shown here.

It was determined that the affected source code places call functions which perform input parameter validation already. It is therefore not needed that a similar safety check is repeated at the call site.

---
 drivers/iommu/msm_iommu.c     | 3 +--
 drivers/iommu/msm_iommu_dev.c | 6 ++----
 2 files changed, 3 insertions(+), 6 deletions(-)

index 6e3dcc28..3e4d888 100644
@@ -73,8 +73,7 @@ fail:
 
 static void __disable_clocks(struct msm_iommu_drvdata *drvdata)
 {
-	if (drvdata->clk)
-		clk_disable(drvdata->clk);
+	clk_disable(drvdata->clk);
 	clk_disable(drvdata->pclk);
 }
 

index 61def7cb..9574d21 100644
@@ -224,8 +224,7 @@ static int msm_iommu_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, drvdata);
 
-	if (iommu_clk)
-		clk_disable(iommu_clk);
+	clk_disable(iommu_clk);
 
 	clk_disable(iommu_pclk);
 

@@ -323,8 +322,7 @@ static int msm_iommu_ctx_probe(struct platform_device *pdev)
 		SET_NSCFG(drvdata->base, mid, 3);
 	}
 
-	if (drvdata->clk)
-		clk_disable(drvdata->clk);
+	clk_disable(drvdata->clk);
 	clk_disable(drvdata->pclk);
 
 	dev_info(&pdev->dev, "context %s using bank %d\n", c->name, c->num);

2.1.2

Applied to arm/msm, thanks.

From: Markus Elfring <[email protected]>
Date: Wed, 22 Oct 2014 20:40:31 +0200
Subject: [PATCH] SCSI-QLA2XXX: Deletion of unnecessary checks before the
function call "vfree"

A semantic patch approach was proposed with the subject "[PATCH with Coccinelle?] Deletion of unnecessary checks before specific function calls" on 2014-03-05.
https://lkml.org/lkml/2014/3/5/344
http://article.gmane.org/gmane.comp.version-control.coccinelle/3513/

This patch pattern application was repeated with the help of the software "Coccinelle 1.0.0-rc22" on the source files for Linux 3.17.1. An extract of the automatically generated update suggestions is shown here.

It was determined that the affected source code places call functions which perform input parameter validation already. It is therefore not needed that a similar safety check is repeated at the call site.

---
 drivers/scsi/qla2xxx/qla_attr.c |  6 ++----
 drivers/scsi/qla2xxx/qla_init.c | 18 ++++++------------
 drivers/scsi/qla2xxx/qla_os.c   |  6 ++----
 3 files changed, 10 insertions(+), 20 deletions(-)

index 82b92c4..95c4c09 100644
@@ -175,10 +175,8 @@ qla2x00_sysfs_write_fw_dump_template(struct file *filp,

struct kobject *kobj,
uint32_t size;

if (off == 0) {
- if (ha->fw_dump)
- vfree(ha->fw_dump);
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump);
+ vfree(ha->fw_dump_template);

ha->fw_dump = NULL;
ha->fw_dump_len = 0;

index a4dde7e..8da3d4f 100644
@@ -5256,8 +5256,7 @@ qla24xx_load_risc_flash(scsi_qla_host_t *vha, uint32_t

*srisc_addr,
if (!IS_QLA27XX(ha))
return rval;

- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5307,8 +5306,7 @@ qla24xx_load_risc_flash(scsi_qla_host_t *vha, uint32_t *srisc_addr,

default_template:
ql_log(ql_log_warn, vha, 0x0168, "Using default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5342,8 +5340,7 @@ default_template:

failed_template:
ql_log(ql_log_warn, vha, 0x016d, "Failed default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;
return rval;
@@ -5559,8 +5556,7 @@ qla24xx_load_risc_blob(scsi_qla_host_t *vha, uint32_t *srisc_addr)
if (!IS_QLA27XX(ha))
return rval;

- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5609,8 +5605,7 @@ qla24xx_load_risc_blob(scsi_qla_host_t *vha, uint32_t *srisc_addr)

default_template:
ql_log(ql_log_warn, vha, 0x0178, "Using default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5644,8 +5639,7 @@ default_template:

failed_template:
ql_log(ql_log_warn, vha, 0x017d, "Failed default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;
return rval;

index db3dbd9..0f9c378 100644
@@ -3676,10 +3676,8 @@ qla2x00_free_fw_dump(struct qla_hw_data *ha)
 		dma_free_coherent(&ha->pdev->dev,
 		    EFT_SIZE, ha->eft, ha->eft_dma);
 
-	if (ha->fw_dump)
-		vfree(ha->fw_dump);
-	if (ha->fw_dump_template)
-		vfree(ha->fw_dump_template);
+	vfree(ha->fw_dump);
+	vfree(ha->fw_dump_template);
 
 	ha->fce = NULL;
 	ha->fce_dma = 0;

2.1.2

I resent the request once more because another "Triple-X" software development adventure might follow ...?

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Wed, 22 Oct 2014 20:40:31 +0200
Subject: [PATCH] SCSI-QLA2XXX: Deletion of unnecessary checks before the
function call "vfree"

A semantic patch approach was proposed with the subject "[PATCH with Coccinelle?] Deletion of unnecessary checks before specific function calls" on 2014-03-05.
https://lkml.org/lkml/2014/3/5/344
http://article.gmane.org/gmane.comp.version-control.coccinelle/3513/

This patch pattern application was repeated with the help of the software "Coccinelle 1.0.0-rc22" on the source files for Linux 3.17.1. An extract of the automatically generated update suggestions is shown here.

It was determined that the affected source code places call functions which perform input parameter validation already. It is therefore not needed that a similar safety check is repeated at the call site.

---
 drivers/scsi/qla2xxx/qla_attr.c |  6 ++----
 drivers/scsi/qla2xxx/qla_init.c | 18 ++++++------------
 drivers/scsi/qla2xxx/qla_os.c   |  6 ++----
 3 files changed, 10 insertions(+), 20 deletions(-)

index 82b92c4..95c4c09 100644
@@ -175,10 +175,8 @@ qla2x00_sysfs_write_fw_dump_template(struct file *filp,

struct kobject *kobj,
uint32_t size;

if (off == 0) {
- if (ha->fw_dump)
- vfree(ha->fw_dump);
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump);
+ vfree(ha->fw_dump_template);

ha->fw_dump = NULL;
ha->fw_dump_len = 0;

index a4dde7e..8da3d4f 100644
@@ -5256,8 +5256,7 @@ qla24xx_load_risc_flash(scsi_qla_host_t *vha, uint32_t

*srisc_addr,
if (!IS_QLA27XX(ha))
return rval;

- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5307,8 +5306,7 @@ qla24xx_load_risc_flash(scsi_qla_host_t *vha, uint32_t *srisc_addr,

default_template:
ql_log(ql_log_warn, vha, 0x0168, "Using default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5342,8 +5340,7 @@ default_template:

failed_template:
ql_log(ql_log_warn, vha, 0x016d, "Failed default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;
return rval;
@@ -5559,8 +5556,7 @@ qla24xx_load_risc_blob(scsi_qla_host_t *vha, uint32_t *srisc_addr)
if (!IS_QLA27XX(ha))
return rval;

- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5609,8 +5605,7 @@ qla24xx_load_risc_blob(scsi_qla_host_t *vha, uint32_t *srisc_addr)

default_template:
ql_log(ql_log_warn, vha, 0x0178, "Using default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;

@@ -5644,8 +5639,7 @@ default_template:

failed_template:
ql_log(ql_log_warn, vha, 0x017d, "Failed default fwdump template\n");
- if (ha->fw_dump_template)
- vfree(ha->fw_dump_template);
+ vfree(ha->fw_dump_template);
ha->fw_dump_template = NULL;
ha->fw_dump_template_len = 0;
return rval;

index db3dbd9..0f9c378 100644
@@ -3676,10 +3676,8 @@ qla2x00_free_fw_dump(struct qla_hw_data *ha)
 		dma_free_coherent(&ha->pdev->dev,
 		    EFT_SIZE, ha->eft, ha->eft_dma);
 
-	if (ha->fw_dump)
-		vfree(ha->fw_dump);
-	if (ha->fw_dump_template)
-		vfree(ha->fw_dump_template);
+	vfree(ha->fw_dump);
+	vfree(ha->fw_dump_template);
 
 	ha->fce = NULL;
 	ha->fce_dma = 0;

2.1.2

From: Markus Elfring <[email protected]>
Date: Thu, 23 Oct 2014 20:55:13 +0200
Subject: [PATCH] staging - rtl8188eu: Deletion of unnecessary checks before
three function calls

The functions kfree(), rtw_free_netdev() and vfree() test whether their argument is NULL and then return immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/staging/rtl8188eu/core/rtw_efuse.c   | 3 +--
 drivers/staging/rtl8188eu/core/rtw_mlme.c    | 3 +--
 drivers/staging/rtl8188eu/core/rtw_sta_mgt.c | 3 +--
 drivers/staging/rtl8188eu/core/rtw_xmit.c    | 6 ++----
 drivers/staging/rtl8188eu/os_dep/usb_intf.c  | 5 ++---
 5 files changed, 7 insertions(+), 13 deletions(-)

b/drivers/staging/rtl8188eu/core/rtw_efuse.c
index 7006088..77f7552 100644

@@ -212,8 +212,7 @@ efuse_phymap_to_logical(u8 *phymap, u16 _offset, u16

_size_byte, u8 *pbuf)
exit:
kfree(efuseTbl);

- if (eFuseWord)
- kfree(eFuseWord);
+ kfree(eFuseWord);
}

static void efuse_read_phymap_from_txpktbuf(

b/drivers/staging/rtl8188eu/core/rtw_mlme.c
index 149c271..df54350 100644

@@ -122,8 +122,7 @@ void rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
 	rtw_free_mlme_priv_ie_data(pmlmepriv);
 
 	if (pmlmepriv) {
-		if (pmlmepriv->free_bss_buf)
-			vfree(pmlmepriv->free_bss_buf);
+		vfree(pmlmepriv->free_bss_buf);
 	}
 }
 

b/drivers/staging/rtl8188eu/core/rtw_sta_mgt.c
index e1dc8fa..af1de9c 100644

@@ -201,8 +201,7 @@ u32	_rtw_free_sta_priv(struct	sta_priv *pstapriv)
 
 		rtw_mfree_sta_priv_lock(pstapriv);
 
-		if (pstapriv->pallocated_stainfo_buf)
-			vfree(pstapriv->pallocated_stainfo_buf);
+		vfree(pstapriv->pallocated_stainfo_buf);
 	}
 
 	return _SUCCESS;

b/drivers/staging/rtl8188eu/core/rtw_xmit.c
index 639ace0..011c9cf 100644

@@ -246,11 +246,9 @@ void _rtw_free_xmit_priv (struct xmit_priv *pxmitpriv)
 		pxmitbuf++;
 	}
 
-	if (pxmitpriv->pallocated_frame_buf)
-		vfree(pxmitpriv->pallocated_frame_buf);
+	vfree(pxmitpriv->pallocated_frame_buf);
 
-	if (pxmitpriv->pallocated_xmitbuf)
-		vfree(pxmitpriv->pallocated_xmitbuf);
+	vfree(pxmitpriv->pallocated_xmitbuf);
 
 	/*  free xmit extension buff */
 	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmit_extbuf;

b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index 407a318..cdb70e4 100644

@@ -456,7 +456,7 @@ free_adapter:
 	if (status != _SUCCESS) {
 		if (pnetdev)
 			rtw_free_netdev(pnetdev);
-		else if (padapter)
+		else
 			vfree(padapter);
 		padapter = NULL;
 	}

@@ -487,8 +487,7 @@ static void rtw_usb_if1_deinit(struct adapter *if1)
 	DBG_88E("+r871xu_dev_remove, hw_init_completed=%d\n",
 		if1->hw_init_completed);
 	rtw_free_drv_sw(if1);
-	if (pnetdev)
-		rtw_free_netdev(pnetdev);
+	rtw_free_netdev(pnetdev);
 }
 
 static int rtw_drv_init(struct usb_interface *pusb_intf, const struct

usb_device_id *pdid)

2.1.2

Why is this here? Please use git send-email to send a patch, so I don't have to hand-edit the text in it.

Also, your Subject is a bit odd, it has a "Re:" in it for no reason, and your From: doesn't match the name you used here.

Please fix up and resend.

thanks,

greg k-h

The functions kfree(), rtw_free_netdev() and vfree() test whether their argument is NULL and then return immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/staging/rtl8188eu/core/rtw_efuse.c   | 3 +--
 drivers/staging/rtl8188eu/core/rtw_mlme.c    | 3 +--
 drivers/staging/rtl8188eu/core/rtw_sta_mgt.c | 3 +--
 drivers/staging/rtl8188eu/core/rtw_xmit.c    | 6 ++----
 drivers/staging/rtl8188eu/os_dep/usb_intf.c  | 5 ++---
 5 files changed, 7 insertions(+), 13 deletions(-)

b/drivers/staging/rtl8188eu/core/rtw_efuse.c
index 7006088..77f7552 100644

@@ -212,8 +212,7 @@ efuse_phymap_to_logical(u8 *phymap, u16 _offset, u16

_size_byte, u8 *pbuf)
exit:
kfree(efuseTbl);

- if (eFuseWord)
- kfree(eFuseWord);
+ kfree(eFuseWord);
}

static void efuse_read_phymap_from_txpktbuf(

b/drivers/staging/rtl8188eu/core/rtw_mlme.c
index 149c271..df54350 100644

@@ -122,8 +122,7 @@ void rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
 	rtw_free_mlme_priv_ie_data(pmlmepriv);
 
 	if (pmlmepriv) {
-		if (pmlmepriv->free_bss_buf)
-			vfree(pmlmepriv->free_bss_buf);
+		vfree(pmlmepriv->free_bss_buf);
 	}
 }
 

b/drivers/staging/rtl8188eu/core/rtw_sta_mgt.c
index e1dc8fa..af1de9c 100644

@@ -201,8 +201,7 @@ u32	_rtw_free_sta_priv(struct	sta_priv *pstapriv)
 
 		rtw_mfree_sta_priv_lock(pstapriv);
 
-		if (pstapriv->pallocated_stainfo_buf)
-			vfree(pstapriv->pallocated_stainfo_buf);
+		vfree(pstapriv->pallocated_stainfo_buf);
 	}
 
 	return _SUCCESS;

b/drivers/staging/rtl8188eu/core/rtw_xmit.c
index 639ace0..011c9cf 100644

@@ -246,11 +246,9 @@ void _rtw_free_xmit_priv (struct xmit_priv *pxmitpriv)
 		pxmitbuf++;
 	}
 
-	if (pxmitpriv->pallocated_frame_buf)
-		vfree(pxmitpriv->pallocated_frame_buf);
+	vfree(pxmitpriv->pallocated_frame_buf);
 
-	if (pxmitpriv->pallocated_xmitbuf)
-		vfree(pxmitpriv->pallocated_xmitbuf);
+	vfree(pxmitpriv->pallocated_xmitbuf);
 
 	/*  free xmit extension buff */
 	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmit_extbuf;

b/drivers/staging/rtl8188eu/os_dep/usb_intf.c
index 407a318..cdb70e4 100644

@@ -456,7 +456,7 @@ free_adapter:
 	if (status != _SUCCESS) {
 		if (pnetdev)
 			rtw_free_netdev(pnetdev);
-		else if (padapter)
+		else
 			vfree(padapter);
 		padapter = NULL;
 	}

@@ -487,8 +487,7 @@ static void rtw_usb_if1_deinit(struct adapter *if1)
 	DBG_88E("+r871xu_dev_remove, hw_init_completed=%d\n",
 		if1->hw_init_completed);
 	rtw_free_drv_sw(if1);
-	if (pnetdev)
-		rtw_free_netdev(pnetdev);
+	rtw_free_netdev(pnetdev);
 }
 
 static int rtw_drv_init(struct usb_interface *pusb_intf, const struct

usb_device_id *pdid)

2.1.2

Are there any chances to achieve the suggested fine-tuning for jump labels also with another semantic patch approach?

Regards,
Markus

No, I don't think so. The pattern is not regular enough.

julia

Now I have got a different impression for corresponding improvement possibilities.

elfring at Sonne:~/Projekte/Linux/stable-patched> spatch.opt -debug -sp-file ~/Projekte/Coccinelle/janitor/move_function_call_before_jump_label1.cocci drivers/staging/rtl8188eu/core/rtw_efuse.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
-----------------------------------------------------------------------
processing semantic patch file: /home/elfring/Projekte/Coccinelle/janitor/move_function_call_before_jump_label1.cocci
with isos from: /usr/local/share/coccinelle/standard.iso
-----------------------------------------------------------------------
@move_function_call_before_jump_label@
expression x;
identifier fu, label;
type t;
@@
t fu(...)
{
... when any
x = kzalloc(...);

  if (x == NULL) {
     ...
     goto label;

}
... when any
+ kfree(x);
label:
- kfree(x);
...
}

HANDLING: drivers/staging/rtl8188eu/core/rtw_efuse.c
-----------------------------------------------------------------------
let's go
-----------------------------------------------------------------------
-----------------------------------------------------------------------
-----------------------------------------------------------------------
move_function_call_before_jump_label =
-----------------------------------------------------------------------
dependencies for rule move_function_call_before_jump_label satisfied:
binding in = []
binding relevant in = []
(ONCE) USING optional_storage builtin isomorphism
transformation info returned:
transform state: 5
with rule_elem:
<<< kfree(move_function_call_before_jump_label:x);
move_function_call_before_jump_label:label:
with binding: [move_function_call_before_jump_label.x --> efuseTbl]
transform state: 204
with rule_elem: -kfree-(-move_function_call_before_jump_label:x-)-; with binding: [move_function_call_before_jump_label.x --> efuseTbl]
binding out = []
transform one node: 204
transform one node: 5
-----------------------------------------------------------------------
Finished
-----------------------------------------------------------------------

diff = 
@@ -209,8 +209,8 @@ efuse_phymap_to_logical(u8 *phymap, u16
        /*  5. Calculate Efuse utilization. */
        /*  */
 
+kfree(efuseTbl);
 exit:
-       kfree(efuseTbl);
 
        kfree(eFuseWord);
 }

Check duplication for 1 files

Can my update suggestion be generalised a bit more for the movement of specific jump labels towards the end of a function implementation like in the use case "efuse_phymap_to_logical()"?

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Wed, 12 Nov 2014 20:42:18 +0100

Another update suggestion was taken into account after patches were applied from static source code analysis.

Markus Elfring (2):
staging: rtl8188eu: Deletion of unnecessary checks before three
function calls
staging: rtl8188eu: Better memory clean-up in efuse_phymap_to_logical()

 drivers/staging/rtl8188eu/core/rtw_efuse.c   | 13 ++++++++-----
 drivers/staging/rtl8188eu/core/rtw_mlme.c    |  3 +--
 drivers/staging/rtl8188eu/core/rtw_sta_mgt.c |  3 +--
 drivers/staging/rtl8188eu/core/rtw_xmit.c    |  6 ++----
 drivers/staging/rtl8188eu/os_dep/usb_intf.c  |  6 +++---
 5 files changed, 15 insertions(+), 16 deletions(-)

2.1.3

From: Markus Elfring <[email protected]>
Date: Wed, 12 Nov 2014 20:25:49 +0100

The functions kfree(), rtw_free_netdev() and vfree() test whether their argument is NULL and then return immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/staging/rtl8188eu/core/rtw_efuse.c   | 3 +--
 drivers/staging/rtl8188eu/core/rtw_mlme.c    | 3 +--
 drivers/staging/rtl8188eu/core/rtw_sta_mgt.c | 3 +--
 drivers/staging/rtl8188eu/core/rtw_xmit.c    | 6 ++----
 drivers/staging/rtl8188eu/os_dep/usb_intf.c  | 6 +++---
 5 files changed, 8 insertions(+), 13 deletions(-)

index 5b997b2..697876b 100644
@@ -212,8 +212,7 @@ efuse_phymap_to_logical(u8 *phymap, u16 _offset, u16 _size_byte, u8  *pbuf)
 exit:
 	kfree(efuseTbl);
 
-	if (eFuseWord)
-		kfree(eFuseWord);
+	kfree(eFuseWord);
 }
 
 static void efuse_read_phymap_from_txpktbuf(

index 149c271..df54350 100644
@@ -122,8 +122,7 @@ void rtw_free_mlme_priv(struct mlme_priv *pmlmepriv)
 	rtw_free_mlme_priv_ie_data(pmlmepriv);
 
 	if (pmlmepriv) {
-		if (pmlmepriv->free_bss_buf)
-			vfree(pmlmepriv->free_bss_buf);
+		vfree(pmlmepriv->free_bss_buf);
 	}
 }
 

index e1dc8fa..af1de9c 100644
@@ -201,8 +201,7 @@ u32	_rtw_free_sta_priv(struct	sta_priv *pstapriv)
 
 		rtw_mfree_sta_priv_lock(pstapriv);
 
-		if (pstapriv->pallocated_stainfo_buf)
-			vfree(pstapriv->pallocated_stainfo_buf);
+		vfree(pstapriv->pallocated_stainfo_buf);
 	}
 
 	return _SUCCESS;

index 639ace0..011c9cf 100644
@@ -246,11 +246,9 @@ void _rtw_free_xmit_priv (struct xmit_priv *pxmitpriv)
 		pxmitbuf++;
 	}
 
-	if (pxmitpriv->pallocated_frame_buf)
-		vfree(pxmitpriv->pallocated_frame_buf);
+	vfree(pxmitpriv->pallocated_frame_buf);
 
-	if (pxmitpriv->pallocated_xmitbuf)
-		vfree(pxmitpriv->pallocated_xmitbuf);
+	vfree(pxmitpriv->pallocated_xmitbuf);
 
 	/*  free xmit extension buff */
 	pxmitbuf = (struct xmit_buf *)pxmitpriv->pxmit_extbuf;

index 407a318..4e2c34b 100644
@@ -456,8 +456,9 @@ free_adapter:
 	if (status != _SUCCESS) {
 		if (pnetdev)
 			rtw_free_netdev(pnetdev);
-		else if (padapter)
+		else
 			vfree(padapter);
+
 		padapter = NULL;
 	}
 exit:

@@ -487,8 +488,7 @@ static void rtw_usb_if1_deinit(struct adapter *if1)
 	DBG_88E("+r871xu_dev_remove, hw_init_completed=%d\n",
 		if1->hw_init_completed);
 	rtw_free_drv_sw(if1);
-	if (pnetdev)
-		rtw_free_netdev(pnetdev);
+	rtw_free_netdev(pnetdev);
 }
 
 static int rtw_drv_init(struct usb_interface *pusb_intf, const struct usb_device_id *pdid)

2.1.3

I find your feedback interesting.

Do you need to adjust your concerns a bit over time when function variants provide a corresponding safety check in their implementations?

Regards,
Markus

There would not seem to be any _need_ to do so. An unnecessary null test is always safe. The only real problem that I can see with an unnecessary null test in error handling code (intrinsically not critical performance wise) is if it gives the illusion that a value can be null when it cannot.

julia

No. Really, free_netdev vs rtw_free_netdev is just an example where it is really bad, but I feel that all of these patches are misguided and harmful.

We should have an if statement if the allocation is optional, we should not have an if statement if the allocation is required.

regards,
dan carpenter

From: Markus Elfring <[email protected]>
Date: Wed, 12 Nov 2014 20:40:12 +0100

Memory releases were handled in an inefficient way by the implementation of the efuse_phymap_to_logical() function in case of an allocation failure. The corresponding clean-up was improved by reordering of kfree() calls and a few adjustments for jump labels.

---
 drivers/staging/rtl8188eu/core/rtw_efuse.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

index 697876b..359f169 100644
@@ -112,7 +112,7 @@ efuse_phymap_to_logical(u8 *phymap, u16 _offset, u16 _size_byte, u8  *pbuf)
 	eFuseWord = (u16 **)rtw_malloc2d(EFUSE_MAX_SECTION_88E, EFUSE_MAX_WORD_UNIT, sizeof(u16));
 	if (eFuseWord == NULL) {
 		DBG_88E("%s: alloc eFuseWord fail!\n", __func__);
-		goto exit;
+		goto cleanup1;
 	}
 
 	/*  0. Refresh efuse init map as all oxFF. */

@@ -130,7 +130,7 @@ efuse_phymap_to_logical(u8 *phymap, u16 _offset, u16 _size_byte, u8  *pbuf)
 		eFuse_Addr++;
 	} else {
 		DBG_88E("EFUSE is empty efuse_Addr-%d efuse_data =%x\n", eFuse_Addr, rtemp8);
-		goto exit;
+		goto cleanup2;
 	}
 
 	/*  */

@@ -209,10 +209,14 @@ efuse_phymap_to_logical(u8 *phymap, u16 _offset, u16 _size_byte, u8  *pbuf)
 	/*  5. Calculate Efuse utilization. */
 	/*  */
 
-exit:
+cleanup2:
+	kfree(eFuseWord);
+
+cleanup1:
 	kfree(efuseTbl);
 
-	kfree(eFuseWord);
+exit:
+	;
 }
 
 static void efuse_read_phymap_from_txpktbuf(

2.1.3

Please fix your email client instead.

Does the document "CodingStyle" need any extensions for special cases? Are there any update candidates in the chapter "7: Centralized exiting of functions"?

Regards,
Markus

I don't understand.

CodingStyle says:

"If there is no cleanup needed then just return directly."

What is not clear about that?

regards,
dan carpenter

Should the naming convention become more explicit for jump labels?

Do you want that I send another update suggestion with other corrections for jump labels in the affected function implementation?

Regards,
Markus

[...]

        kfree(eFuseWord);

	kfree(efuseTbl);

Thanks for your reminder.

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/drivers/staging/rtl8188eu/core/rtw_efuse.c?id=3cfab18ce55282a85e2c7e5db15c5daf065efdb4

Regards,
Markus

From: Markus Elfring <[email protected]>
Date: Mon, 25 Jul 2016 22:20:24 +0200

The vfree() function performs also input parameter validation. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/staging/rtl8188eu/core/rtw_xmit.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index e0a5567..39ffb46 100644
@@ -252,10 +252,7 @@ void _rtw_free_xmit_priv(struct xmit_priv *pxmitpriv)
 		pxmitbuf++;
 	}
 
-	if (pxmitpriv->pallocated_xmit_extbuf) {
-		vfree(pxmitpriv->pallocated_xmit_extbuf);
-	}
-
+	vfree(pxmitpriv->pallocated_xmit_extbuf);
 	rtw_free_hwxmits(padapter);
 
 	mutex_destroy(&pxmitpriv->ack_tx_mutex);

2.9.2

The functions debug_unregister() and kfree_fsm() test whether their argument is NULL and then return immediately. Thus the test around the call is not needed.

This issue was detected by using the Coccinelle software.

---
 drivers/s390/net/claw.c      |  6 ++----
 drivers/s390/net/ctcm_main.c |  6 ++----
 drivers/s390/net/lcs.c       |  6 ++----
 drivers/s390/net/netiucv.c   | 12 ++++--------
 4 files changed, 10 insertions(+), 20 deletions(-)

index 213e54e..d609ca0 100644
@@ -109,10 +109,8 @@ static debug_info_t *claw_dbf_trace;
 static void
 claw_unregister_debug_facility(void)
 {
-	if (claw_dbf_setup)
-		debug_unregister(claw_dbf_setup);
-	if (claw_dbf_trace)
-		debug_unregister(claw_dbf_trace);
+	debug_unregister(claw_dbf_setup);
+	debug_unregister(claw_dbf_trace);
 }
 
 static int

index e056dd4..34dc0f3 100644
@@ -1074,8 +1074,7 @@ static void ctcm_free_netdevice(struct net_device *dev)
 	if (priv) {
 		grp = priv->mpcg;
 		if (grp) {
-			if (grp->fsm)
-				kfree_fsm(grp->fsm);
+			kfree_fsm(grp->fsm);
 			if (grp->xid_skb)
 				dev_kfree_skb(grp->xid_skb);
 			if (grp->rcvd_xid_skb)

@@ -1672,8 +1671,7 @@ static int ctcm_shutdown_device(struct ccwgroup_device *cgdev)
 		ctcm_free_netdevice(dev);
 	}
 
-	if (priv->fsm)
-		kfree_fsm(priv->fsm);
+	kfree_fsm(priv->fsm);
 
 	ccw_device_set_offline(cgdev->cdev[1]);
 	ccw_device_set_offline(cgdev->cdev[0]);

index 0a7d87c..5dfa7dd 100644
@@ -88,10 +88,8 @@ static debug_info_t *lcs_dbf_trace;
 static void
 lcs_unregister_debug_facility(void)
 {
-	if (lcs_dbf_setup)
-		debug_unregister(lcs_dbf_setup);
-	if (lcs_dbf_trace)
-		debug_unregister(lcs_dbf_trace);
+	debug_unregister(lcs_dbf_setup);
+	debug_unregister(lcs_dbf_trace);
 }
 
 static int

index 0a87809..bdcc3fe 100644
@@ -487,12 +487,9 @@ DEFINE_PER_CPU(char[256], iucv_dbf_txt_buf);
 
 static void iucv_unregister_dbf_views(void)
 {
-	if (iucv_dbf_setup)
-		debug_unregister(iucv_dbf_setup);
-	if (iucv_dbf_data)
-		debug_unregister(iucv_dbf_data);
-	if (iucv_dbf_trace)
-		debug_unregister(iucv_dbf_trace);
+	debug_unregister(iucv_dbf_setup);
+	debug_unregister(iucv_dbf_data);
+	debug_unregister(iucv_dbf_trace);
 }
 static int iucv_register_dbf_views(void)
 {

@@ -1975,8 +1972,7 @@ static void netiucv_free_netdevice(struct net_device *dev)
 	if (privptr) {
 		if (privptr->conn)
 			netiucv_remove_connection(privptr->conn);
-		if (privptr->fsm)
-			kfree_fsm(privptr->fsm);
+		kfree_fsm(privptr->fsm);
 		privptr->conn = NULL; privptr->fsm = NULL;
 		/* privptr gets freed by free_netdev() */
 	}

2.1.3

This one is buggy.

I'm sorry, but please stop sending these.

For kfree(), at least we all know that kfree() accepts NULL pointer. But for this one:
1) I don't know what the functions do so I have to look at the code.
2) It's in a arch that I don't compile so cscope isn't set up meaning it's hard to find the functions.

You're sending a lot of patches and they are all hard to review and some of them are buggy and none of them really add any value. It's a waste of your time and it's a waste of my time.

regards,
dan carpenter

I am still interested to clarify this opinion a bit more.

I am going to improve more implementation details in affected source files.

I hope that static source code analysis can help here.

Do you find the Coccinelle software also useful for your area?

Thanks for your feedback.

The suggested source code clean-up might result in a measurable effect depending on the call frequency for the changed functions. Can I help you in any ways to make corresponding review easier?

It can be your choice to reject my update suggestion.

Regards,
Markus

After your patch then it will print warning messages.

The truth is I think that all these patches are bad and they make the code harder to read.

Before: The code is clear and there is no NULL dereference.
After: You have to remember that rtw_free_netdev() accepts NULL
pointers but free_netdev() does not accept NULL pointers.

The if statements are there for *human* readers to understand and you are making it harder for humans to understand the code.

Even for kfree(), just removing the if statement is not really the right fix. We do it because everyone knows kfree(), but what Julia Lawall said is the real correct way change the code and make it simpler for people to understand:

https://lkml.org/lkml/2014/10/31/452

I know it's fun to send automated patches but these make the code worse and they waste reviewer time.

regards,
dan carpenter

To which messages do you refer to?

Where do you stumble on a null pointer access?

Are any improvements needed for the corresponding documentation to make it better accessible besides the source code?

Is there a target conflict between source code understandability and software efficiency?

You refer to another update suggestion for the software area "staging: rtl8188eu".
Do you find adjustments for jump labels easier to accept than the simple deletion of specific null pointer checks?

I hope that small automated changes can also help to improve affected source files.

Regards,
Markus

When people are writing or reading code, they will not necessarily look at the documentation for every function that they use.

Efficiency is not an issue. This code is all in rare error handling paths or in service removal functions. None of it is in a critical path. What is important is to be able to easily check that what needs to be done is actually done. Removing null tests makes it more obscure what needs to be done, because it means that the conditions under which a function needs to be called (which may be different than the conditions under which it can be called) are less apparent.

julia

Open your eyeballs up and read the code.

I'm not talking about bugs, I'm talking about code clarity. Before is more clear than after.

Documentation doesn't reduce the number of things to remember it just documents it. Meanwhile if we leave the code as-is there is no need for documentation because the code is clear.

If you can benchmark the code and the new code is faster then, yes, this patch is good and we will apply it. If you have no benchmarks then do not send the patch.

Yes.

No. The changes make the code less clear.

regards,
dan carpenter

I guess that I do not have enough resources myself to measure different run time effects in a S390 environment.

Are other software developers and testers eventually interested to try a few pointer check adjustments out a bit more?

Regards,
Markus

Dan

I disagree, I think the code requires less effort to read without the if test.

A developer reading the code will wonder why kfree does not handle the case when its argument is NULL. This takes effort.

Now there might be a reason while kfree (or any other function) does not handle NULL, in which case the test is necessary for that reason. Or perhaps calling kfree has other consequences and this means it is good to minimise the number of calls, fair enough.

The reverse is true.

But if there are other reasons, then leave the test in.

Derek M. Jones                  tel: +44 (0) 1252 520 667
Knowledge Software Ltd          blog:shape-of-code.coding-guidelines.com
Software analysis               http://www.knosof.co.uk

This may be true in general, but the standard assumption about kernel functions is that they are not defensive. Everything used in the kernel is defined in the kernel, and is normally written in a way to be optimal for in-kernel usage.

The point about a null test is that it makes it apparent at the current point that the value can be null. The default assumption is that it cannot, ie that functions are only called when doing so is useful.

Actually, this assumption can be exploited by automatic tools for finding out what needs to be done when:

Suman Saha, Jean-Pierre Lozi, Ga?l Thomas, Julia L. Lawall, Gilles Muller:
Hector: Detecting Resource-Release Omission Faults in error-handling code for systems software. DSN 2013: 1-12

The point is that Linux code contains a huge number of alloc and free functions, many of which are not very well known. If free functions are only called when they are useful, then you can infer which free functions go with which alloc functions. If all free functions are just lumped together and called at random, then you lose a lot of information.

Similarly, as a human, when I look at code I don't know, it is very confusing to have functions called when as far as I can tell they don't need to be. A bunch of validity tests at the beginning of the called function doesn't really help, because as Dan points out the code is not always easy to find, and in the function definition itself one doesn't know in what contexts that code is intended to be used.

In separate cleanup functions (destroy_xxx) perhaps the tests are more or less noise. But in the error handling code of an initialization function, no test means to me that the call is needed at the current point, and a test means to me that for some reason it is not statically known whether a call is needed or not. This is information that is really useful for understanding the code, for both humans and for tools.

julia

Does it make a difference for you where such safety checks should be finally placed in the source code?

Do you consider different assumptions for function implementations from an operating system in comparison to user applications?

How much will corresponding expectations need adjustments with the software evolution?

Regards,
Markus

I went digging through some of Markus's old patch history, and noticed this...

I'm tending to concur.

And you're still sending buggy patches that exhibit the same qualities. They're still wasting Dan's time, and now they're wasting mine.

I appreciate automated checkers where they provide added value, but I really feel you haven't done your diligence on them.

Regards,
Brian

I agree with your proposed debug_unregister() changes, but not with your kfree_fsm() change.

Regards, Ursula Braun