[PATCH] UBIFS: Remove some dead code

[PATCH] UBIFS: Remove some dead code

[Christophe JAILLET]

'ubifs_fast_find_freeable()' can not return an error pointer, so this test
can be removed.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 fs/ubifs/gc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
index e845c64b6ce1..7b35e3d6cde7 100644
--- a/fs/ubifs/gc.c
+++ b/fs/ubifs/gc.c
@@ -846,10 +846,6 @@ int ubifs_gc_start_commit(struct ubifs_info *c)
 	 */
 	while (1) {
 		lp = ubifs_fast_find_freeable(c);
-		if (IS_ERR(lp)) {
-			err = PTR_ERR(lp);
-			goto out;
-		}
 		if (!lp)
 			break;
 		ubifs_assert(!(lp->flags & LPROPS_TAKEN));
-- 
2.9.3

[PATCH] UBIFS: Remove some dead code

[Christophe JAILLET]

'ubifs_fast_find_freeable()' can not return an error pointer, so this test
can be removed.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 fs/ubifs/gc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
index e845c64b6ce1..7b35e3d6cde7 100644
--- a/fs/ubifs/gc.c
+++ b/fs/ubifs/gc.c
@@ -846,10 +846,6 @@ int ubifs_gc_start_commit(struct ubifs_info *c)
 	 */
 	while (1) {
 		lp = ubifs_fast_find_freeable(c);
-		if (IS_ERR(lp)) {
-			err = PTR_ERR(lp);
-			goto out;
-		}
 		if (!lp)
 			break;
 		ubifs_assert(!(lp->flags & LPROPS_TAKEN));
-- 
2.9.3

Re: [PATCH] UBIFS: Remove some dead code

[Richard Weinberger]

On 01.11.2016 07:45, Christophe JAILLET wrote:
> 'ubifs_fast_find_freeable()' can not return an error pointer, so this test
> can be removed.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  fs/ubifs/gc.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
> index e845c64b6ce1..7b35e3d6cde7 100644
> --- a/fs/ubifs/gc.c
> +++ b/fs/ubifs/gc.c
> @@ -846,10 +846,6 @@ int ubifs_gc_start_commit(struct ubifs_info *c)
>  	 */
>  	while (1) {
>  		lp = ubifs_fast_find_freeable(c);
> -		if (IS_ERR(lp)) {
> -			err = PTR_ERR(lp);
> -			goto out;
> -		}

Good catch, how did you find this?
If you have a tool/script I'd like to use it too.

Thanks,
//richard

Re: [PATCH] UBIFS: Remove some dead code

[Richard Weinberger]

On 01.11.2016 07:45, Christophe JAILLET wrote:
> 'ubifs_fast_find_freeable()' can not return an error pointer, so this test
> can be removed.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  fs/ubifs/gc.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
> index e845c64b6ce1..7b35e3d6cde7 100644
> --- a/fs/ubifs/gc.c
> +++ b/fs/ubifs/gc.c
> @@ -846,10 +846,6 @@ int ubifs_gc_start_commit(struct ubifs_info *c)
>  	 */
>  	while (1) {
>  		lp = ubifs_fast_find_freeable(c);
> -		if (IS_ERR(lp)) {
> -			err = PTR_ERR(lp);
> -			goto out;
> -		}

Good catch, how did you find this?
If you have a tool/script I'd like to use it too.

Thanks,
//richard

Re: [PATCH] UBIFS: Remove some dead code

[Christophe JAILLET]

Le 01/11/2016 à 12:42, Richard Weinberger a écrit :
> On 01.11.2016 07:45, Christophe JAILLET wrote:
>> 'ubifs_fast_find_freeable()' can not return an error pointer, so this test
>> can be removed.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   fs/ubifs/gc.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
>> index e845c64b6ce1..7b35e3d6cde7 100644
>> --- a/fs/ubifs/gc.c
>> +++ b/fs/ubifs/gc.c
>> @@ -846,10 +846,6 @@ int ubifs_gc_start_commit(struct ubifs_info *c)
>>   	 */
>>   	while (1) {
>>   		lp = ubifs_fast_find_freeable(c);
>> -		if (IS_ERR(lp)) {
>> -			err = PTR_ERR(lp);
>> -			goto out;
>> -		}
> Good catch, how did you find this?
> If you have a tool/script I'd like to use it too.
>
> Thanks,
> //richard
> --

Hi,
well, it is a bit tricky.

AFAIK, coccinelle is only able to match things in a given file. Finding 
issues between 2 files can be tricky.

So first, I have built a list a functions which are likely to return 
NULL, either because they explicitly return NULL or if its return value 
is tested against NULL or not. See coccinelle script n°1 below.
Then I have built a list of functions followed by a test with IS_ERR. 
See coccinelle script n°2 below.

These 2 scripts generate 2 lists of functions.
If a function is present in the 2 files, it is likely that something is 
spurious.

Either the IS_ERR is not needed (this is the case in the patch above), 
either the return value is incorrectly checked. Could also be that NULL 
is returned but an error pointer would be a better option.


I also did more or less the same for functions that return PTR_ERR and 
functions that are not followed by a test with IS_ERR.
I can post these other scripts if wanted.



Any ideas to improve or speed-up the coccinelle scripts are welcome.
Julia ?


Best regards,
CJ



Coccinelle script n°1:
=====================
@find@
identifier f;
@@

    f(...)
    {
       ...
       return NULL;
    }

@script:python@
f << find.f;
@@

print "%s" %(f)



@find2@
identifier f;
expression x;
statement S;
@@

    x = f(...);
(
    if (x) S
|
    if (!x) S
)

@script:python@
f << find2.f;
@@

print "%s" %(f)





Coccinelle script n°2:
=====================
@find@
statement S;
type t;
t *x;
identifier f;
@@

     x = f(...);
(
     if (IS_ERR(x)) S
|
     if (!IS_ERR(x)) S
)



@script:python@
f << find.f;
@@

print "%s" %(f)

Re: [PATCH] UBIFS: Remove some dead code

[Christophe JAILLET]

Le 01/11/2016 à 12:42, Richard Weinberger a écrit :
> On 01.11.2016 07:45, Christophe JAILLET wrote:
>> 'ubifs_fast_find_freeable()' can not return an error pointer, so this test
>> can be removed.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   fs/ubifs/gc.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
>> index e845c64b6ce1..7b35e3d6cde7 100644
>> --- a/fs/ubifs/gc.c
>> +++ b/fs/ubifs/gc.c
>> @@ -846,10 +846,6 @@ int ubifs_gc_start_commit(struct ubifs_info *c)
>>   	 */
>>   	while (1) {
>>   		lp = ubifs_fast_find_freeable(c);
>> -		if (IS_ERR(lp)) {
>> -			err = PTR_ERR(lp);
>> -			goto out;
>> -		}
> Good catch, how did you find this?
> If you have a tool/script I'd like to use it too.
>
> Thanks,
> //richard
> --

Hi,
well, it is a bit tricky.

AFAIK, coccinelle is only able to match things in a given file. Finding 
issues between 2 files can be tricky.

So first, I have built a list a functions which are likely to return 
NULL, either because they explicitly return NULL or if its return value 
is tested against NULL or not. See coccinelle script n°1 below.
Then I have built a list of functions followed by a test with IS_ERR. 
See coccinelle script n°2 below.

These 2 scripts generate 2 lists of functions.
If a function is present in the 2 files, it is likely that something is 
spurious.

Either the IS_ERR is not needed (this is the case in the patch above), 
either the return value is incorrectly checked. Could also be that NULL 
is returned but an error pointer would be a better option.


I also did more or less the same for functions that return PTR_ERR and 
functions that are not followed by a test with IS_ERR.
I can post these other scripts if wanted.



Any ideas to improve or speed-up the coccinelle scripts are welcome.
Julia ?


Best regards,
CJ



Coccinelle script n°1:
==========@find@
identifier f;
@@

    f(...)
    {
       ...
       return NULL;
    }

@script:python@
f << find.f;
@@

print "%s" %(f)



@find2@
identifier f;
expression x;
statement S;
@@

    x = f(...);
(
    if (x) S
|
    if (!x) S
)

@script:python@
f << find2.f;
@@

print "%s" %(f)





Coccinelle script n°2:
==========@find@
statement S;
type t;
t *x;
identifier f;
@@

     x = f(...);
(
     if (IS_ERR(x)) S
|
     if (!IS_ERR(x)) S
)



@script:python@
f << find.f;
@@

print "%s" %(f)



--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] UBIFS: Remove some dead code

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2745 bytes --]



On Tue, 1 Nov 2016, Christophe JAILLET wrote:

> Le 01/11/2016 à 12:42, Richard Weinberger a écrit :
> > On 01.11.2016 07:45, Christophe JAILLET wrote:
> > > 'ubifs_fast_find_freeable()' can not return an error pointer, so this test
> > > can be removed.
> > >
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > >   fs/ubifs/gc.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > >
> > > diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
> > > index e845c64b6ce1..7b35e3d6cde7 100644
> > > --- a/fs/ubifs/gc.c
> > > +++ b/fs/ubifs/gc.c
> > > @@ -846,10 +846,6 @@ int ubifs_gc_start_commit(struct ubifs_info *c)
> > >   	 */
> > >   	while (1) {
> > >   		lp = ubifs_fast_find_freeable(c);
> > > -		if (IS_ERR(lp)) {
> > > -			err = PTR_ERR(lp);
> > > -			goto out;
> > > -		}
> > Good catch, how did you find this?
> > If you have a tool/script I'd like to use it too.
> >
> > Thanks,
> > //richard
> > --
>
> Hi,
> well, it is a bit tricky.
>
> AFAIK, coccinelle is only able to match things in a given file. Finding issues
> between 2 files can be tricky.
>
> So first, I have built a list a functions which are likely to return NULL,
> either because they explicitly return NULL or if its return value is tested
> against NULL or not. See coccinelle script n°1 below.
> Then I have built a list of functions followed by a test with IS_ERR. See
> coccinelle script n°2 below.
>
> These 2 scripts generate 2 lists of functions.
> If a function is present in the 2 files, it is likely that something is
> spurious.
>
> Either the IS_ERR is not needed (this is the case in the patch above), either
> the return value is incorrectly checked. Could also be that NULL is returned
> but an error pointer would be a better option.
>
>
> I also did more or less the same for functions that return PTR_ERR and
> functions that are not followed by a test with IS_ERR.
> I can post these other scripts if wanted.
>
>
>
> Any ideas to improve or speed-up the coccinelle scripts are welcome.
> Julia ?

I made a combination of an OCaml program and a Coccinelle script to
collect the error codes (-ENOMEM, etc) that a function is returning, fully
interprocedurally throughout the kernel.  I think it ran for 17 iterations
until reaching a fixed point.  For the information I collected, it ran in
a few hours on an 8 core machine.  I think it could be repurposed to address
this NULL vs ERR_PTR problem.  I'm rolling through the Dutch countryside at
the moment, but I could take a look tomorrow.  Without going to a full
fixpoint iteration, the above strategy looks reasonable.  It could be
interesting to compare the results.  My fixpoint strategy gives up on
function pointers, so it is not completely accurate either.

julia

Re: [PATCH] UBIFS: Remove some dead code

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2819 bytes --]



On Tue, 1 Nov 2016, Christophe JAILLET wrote:

> Le 01/11/2016 à 12:42, Richard Weinberger a écrit :
> > On 01.11.2016 07:45, Christophe JAILLET wrote:
> > > 'ubifs_fast_find_freeable()' can not return an error pointer, so this test
> > > can be removed.
> > >
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > >   fs/ubifs/gc.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > >
> > > diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
> > > index e845c64b6ce1..7b35e3d6cde7 100644
> > > --- a/fs/ubifs/gc.c
> > > +++ b/fs/ubifs/gc.c
> > > @@ -846,10 +846,6 @@ int ubifs_gc_start_commit(struct ubifs_info *c)
> > >   	 */
> > >   	while (1) {
> > >   		lp = ubifs_fast_find_freeable(c);
> > > -		if (IS_ERR(lp)) {
> > > -			err = PTR_ERR(lp);
> > > -			goto out;
> > > -		}
> > Good catch, how did you find this?
> > If you have a tool/script I'd like to use it too.
> >
> > Thanks,
> > //richard
> > --
>
> Hi,
> well, it is a bit tricky.
>
> AFAIK, coccinelle is only able to match things in a given file. Finding issues
> between 2 files can be tricky.
>
> So first, I have built a list a functions which are likely to return NULL,
> either because they explicitly return NULL or if its return value is tested
> against NULL or not. See coccinelle script n°1 below.
> Then I have built a list of functions followed by a test with IS_ERR. See
> coccinelle script n°2 below.
>
> These 2 scripts generate 2 lists of functions.
> If a function is present in the 2 files, it is likely that something is
> spurious.
>
> Either the IS_ERR is not needed (this is the case in the patch above), either
> the return value is incorrectly checked. Could also be that NULL is returned
> but an error pointer would be a better option.
>
>
> I also did more or less the same for functions that return PTR_ERR and
> functions that are not followed by a test with IS_ERR.
> I can post these other scripts if wanted.
>
>
>
> Any ideas to improve or speed-up the coccinelle scripts are welcome.
> Julia ?

I made a combination of an OCaml program and a Coccinelle script to
collect the error codes (-ENOMEM, etc) that a function is returning, fully
interprocedurally throughout the kernel.  I think it ran for 17 iterations
until reaching a fixed point.  For the information I collected, it ran in
a few hours on an 8 core machine.  I think it could be repurposed to address
this NULL vs ERR_PTR problem.  I'm rolling through the Dutch countryside at
the moment, but I could take a look tomorrow.  Without going to a full
fixpoint iteration, the above strategy looks reasonable.  It could be
interesting to compare the results.  My fixpoint strategy gives up on
function pointers, so it is not completely accurate either.

julia

Re: [PATCH] UBIFS: Remove some dead code

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3379 bytes --]



On Tue, 1 Nov 2016, Christophe JAILLET wrote:

> Le 01/11/2016 à 12:42, Richard Weinberger a écrit :
> > On 01.11.2016 07:45, Christophe JAILLET wrote:
> > > 'ubifs_fast_find_freeable()' can not return an error pointer, so this test
> > > can be removed.
> > >
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > >   fs/ubifs/gc.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > >
> > > diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
> > > index e845c64b6ce1..7b35e3d6cde7 100644
> > > --- a/fs/ubifs/gc.c
> > > +++ b/fs/ubifs/gc.c
> > > @@ -846,10 +846,6 @@ int ubifs_gc_start_commit(struct ubifs_info *c)
> > >   	 */
> > >   	while (1) {
> > >   		lp = ubifs_fast_find_freeable(c);
> > > -		if (IS_ERR(lp)) {
> > > -			err = PTR_ERR(lp);
> > > -			goto out;
> > > -		}
> > Good catch, how did you find this?
> > If you have a tool/script I'd like to use it too.
> >
> > Thanks,
> > //richard
> > --
>
> Hi,
> well, it is a bit tricky.
>
> AFAIK, coccinelle is only able to match things in a given file. Finding issues
> between 2 files can be tricky.
>
> So first, I have built a list a functions which are likely to return NULL,
> either because they explicitly return NULL or if its return value is tested
> against NULL or not. See coccinelle script n°1 below.
> Then I have built a list of functions followed by a test with IS_ERR. See
> coccinelle script n°2 below.
>
> These 2 scripts generate 2 lists of functions.
> If a function is present in the 2 files, it is likely that something is
> spurious.
>
> Either the IS_ERR is not needed (this is the case in the patch above), either
> the return value is incorrectly checked. Could also be that NULL is returned
> but an error pointer would be a better option.
>
>
> I also did more or less the same for functions that return PTR_ERR and
> functions that are not followed by a test with IS_ERR.
> I can post these other scripts if wanted.
>
>
>
> Any ideas to improve or speed-up the coccinelle scripts are welcome.
> Julia ?
>
>
> Best regards,
> CJ
>
>
>
> Coccinelle script n°1:
> =====================
> @find@
> identifier f;
> @@
>
>    f(...)
>    {
>       ...
>       return NULL;

Do you want functions that always return NULL or that may return NULL?
Currently you are getting those that always return NULL, except perhaps
under ifs.  But if you want functions that may return NULL, it would be
better to put exists in the rule header after find.  This will also likely
be more efficient.

>    }
>
> @script:python@
> f << find.f;
> @@
>
> print "%s" %(f)
>
>
>
> @find2@
> identifier f;
> expression x;
> statement S;
> @@
>
>    x = f(...);
> (
>    if (x) S
> |
>    if (!x) S
> )

You might want to put ... after x = f(...);, or rather .... when != x = e
for some expression metavariable e.

>
> @script:python@
> f << find2.f;
> @@
>
> print "%s" %(f)
>
>
>
>
>
> Coccinelle script n°2:
> =====================
> @find@
> statement S;
> type t;
> t *x;
> identifier f;
> @@
>
>     x = f(...);

Same here about the ...

julia

> (
>     if (IS_ERR(x)) S
> |
>     if (!IS_ERR(x)) S
> )
>
>
>
> @script:python@
> f << find.f;
> @@
>
> print "%s" %(f)
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] UBIFS: Remove some dead code

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3538 bytes --]



On Tue, 1 Nov 2016, Christophe JAILLET wrote:

> Le 01/11/2016 à 12:42, Richard Weinberger a écrit :
> > On 01.11.2016 07:45, Christophe JAILLET wrote:
> > > 'ubifs_fast_find_freeable()' can not return an error pointer, so this test
> > > can be removed.
> > >
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > >   fs/ubifs/gc.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > >
> > > diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
> > > index e845c64b6ce1..7b35e3d6cde7 100644
> > > --- a/fs/ubifs/gc.c
> > > +++ b/fs/ubifs/gc.c
> > > @@ -846,10 +846,6 @@ int ubifs_gc_start_commit(struct ubifs_info *c)
> > >   	 */
> > >   	while (1) {
> > >   		lp = ubifs_fast_find_freeable(c);
> > > -		if (IS_ERR(lp)) {
> > > -			err = PTR_ERR(lp);
> > > -			goto out;
> > > -		}
> > Good catch, how did you find this?
> > If you have a tool/script I'd like to use it too.
> >
> > Thanks,
> > //richard
> > --
>
> Hi,
> well, it is a bit tricky.
>
> AFAIK, coccinelle is only able to match things in a given file. Finding issues
> between 2 files can be tricky.
>
> So first, I have built a list a functions which are likely to return NULL,
> either because they explicitly return NULL or if its return value is tested
> against NULL or not. See coccinelle script n°1 below.
> Then I have built a list of functions followed by a test with IS_ERR. See
> coccinelle script n°2 below.
>
> These 2 scripts generate 2 lists of functions.
> If a function is present in the 2 files, it is likely that something is
> spurious.
>
> Either the IS_ERR is not needed (this is the case in the patch above), either
> the return value is incorrectly checked. Could also be that NULL is returned
> but an error pointer would be a better option.
>
>
> I also did more or less the same for functions that return PTR_ERR and
> functions that are not followed by a test with IS_ERR.
> I can post these other scripts if wanted.
>
>
>
> Any ideas to improve or speed-up the coccinelle scripts are welcome.
> Julia ?
>
>
> Best regards,
> CJ
>
>
>
> Coccinelle script n°1:
> =====================
> @find@
> identifier f;
> @@
>
>    f(...)
>    {
>       ...
>       return NULL;

Do you want functions that always return NULL or that may return NULL?
Currently you are getting those that always return NULL, except perhaps
under ifs.  But if you want functions that may return NULL, it would be
better to put exists in the rule header after find.  This will also likely
be more efficient.

>    }
>
> @script:python@
> f << find.f;
> @@
>
> print "%s" %(f)
>
>
>
> @find2@
> identifier f;
> expression x;
> statement S;
> @@
>
>    x = f(...);
> (
>    if (x) S
> |
>    if (!x) S
> )

You might want to put ... after x = f(...);, or rather .... when != x = e
for some expression metavariable e.

>
> @script:python@
> f << find2.f;
> @@
>
> print "%s" %(f)
>
>
>
>
>
> Coccinelle script n°2:
> =====================
> @find@
> statement S;
> type t;
> t *x;
> identifier f;
> @@
>
>     x = f(...);

Same here about the ...

julia

> (
>     if (IS_ERR(x)) S
> |
>     if (!IS_ERR(x)) S
> )
>
>
>
> @script:python@
> f << find.f;
> @@
>
> print "%s" %(f)
>
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] UBIFS: Remove some dead code

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2875 bytes --]



On Tue, 1 Nov 2016, Christophe JAILLET wrote:

> Le 01/11/2016 à 12:42, Richard Weinberger a écrit :
> > On 01.11.2016 07:45, Christophe JAILLET wrote:
> > > 'ubifs_fast_find_freeable()' can not return an error pointer, so this test
> > > can be removed.
> > >
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > >   fs/ubifs/gc.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > >
> > > diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
> > > index e845c64b6ce1..7b35e3d6cde7 100644
> > > --- a/fs/ubifs/gc.c
> > > +++ b/fs/ubifs/gc.c
> > > @@ -846,10 +846,6 @@ int ubifs_gc_start_commit(struct ubifs_info *c)
> > >   	 */
> > >   	while (1) {
> > >   		lp = ubifs_fast_find_freeable(c);
> > > -		if (IS_ERR(lp)) {
> > > -			err = PTR_ERR(lp);
> > > -			goto out;
> > > -		}
> > Good catch, how did you find this?
> > If you have a tool/script I'd like to use it too.
> >
> > Thanks,
> > //richard
> > --
>
> Hi,
> well, it is a bit tricky.
>
> AFAIK, coccinelle is only able to match things in a given file. Finding issues
> between 2 files can be tricky.
>
> So first, I have built a list a functions which are likely to return NULL,
> either because they explicitly return NULL or if its return value is tested
> against NULL or not. See coccinelle script n°1 below.
> Then I have built a list of functions followed by a test with IS_ERR. See
> coccinelle script n°2 below.
>
> These 2 scripts generate 2 lists of functions.
> If a function is present in the 2 files, it is likely that something is
> spurious.
>
> Either the IS_ERR is not needed (this is the case in the patch above), either
> the return value is incorrectly checked. Could also be that NULL is returned
> but an error pointer would be a better option.
>
>
> I also did more or less the same for functions that return PTR_ERR and
> functions that are not followed by a test with IS_ERR.
> I can post these other scripts if wanted.
>
>
>
> Any ideas to improve or speed-up the coccinelle scripts are welcome.
> Julia ?
>
>
> Best regards,
> CJ
>
>
>
> Coccinelle script n°1:
> =====================
> @find@
> identifier f;
> @@
>
>    f(...)
>    {
>       ...
>       return NULL;
>    }
>
> @script:python@
> f << find.f;
> @@
>
> print "%s" %(f)
>
>
>
> @find2@
> identifier f;
> expression x;
> statement S;
> @@
>
>    x = f(...);
> (
>    if (x) S
> |
>    if (!x) S
> )
>
> @script:python@
> f << find2.f;
> @@
>
> print "%s" %(f)
>
>
>
>
>
> Coccinelle script n°2:
> =====================
> @find@
> statement S;
> type t;
> t *x;
> identifier f;
> @@
>
>     x = f(...);
> (
>     if (IS_ERR(x)) S
> |
>     if (!IS_ERR(x)) S
> )
>
>
>
> @script:python@
> f << find.f;
> @@
>
> print "%s" %(f)

Shouldn't there be a rule like the return NULL here too?

You may also want to take into account the possibility of

ret = NULL;
... when != ret = e
return ret;

julia

Re: [PATCH] UBIFS: Remove some dead code

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3019 bytes --]



On Tue, 1 Nov 2016, Christophe JAILLET wrote:

> Le 01/11/2016 à 12:42, Richard Weinberger a écrit :
> > On 01.11.2016 07:45, Christophe JAILLET wrote:
> > > 'ubifs_fast_find_freeable()' can not return an error pointer, so this test
> > > can be removed.
> > >
> > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > > ---
> > >   fs/ubifs/gc.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > >
> > > diff --git a/fs/ubifs/gc.c b/fs/ubifs/gc.c
> > > index e845c64b6ce1..7b35e3d6cde7 100644
> > > --- a/fs/ubifs/gc.c
> > > +++ b/fs/ubifs/gc.c
> > > @@ -846,10 +846,6 @@ int ubifs_gc_start_commit(struct ubifs_info *c)
> > >   	 */
> > >   	while (1) {
> > >   		lp = ubifs_fast_find_freeable(c);
> > > -		if (IS_ERR(lp)) {
> > > -			err = PTR_ERR(lp);
> > > -			goto out;
> > > -		}
> > Good catch, how did you find this?
> > If you have a tool/script I'd like to use it too.
> >
> > Thanks,
> > //richard
> > --
>
> Hi,
> well, it is a bit tricky.
>
> AFAIK, coccinelle is only able to match things in a given file. Finding issues
> between 2 files can be tricky.
>
> So first, I have built a list a functions which are likely to return NULL,
> either because they explicitly return NULL or if its return value is tested
> against NULL or not. See coccinelle script n°1 below.
> Then I have built a list of functions followed by a test with IS_ERR. See
> coccinelle script n°2 below.
>
> These 2 scripts generate 2 lists of functions.
> If a function is present in the 2 files, it is likely that something is
> spurious.
>
> Either the IS_ERR is not needed (this is the case in the patch above), either
> the return value is incorrectly checked. Could also be that NULL is returned
> but an error pointer would be a better option.
>
>
> I also did more or less the same for functions that return PTR_ERR and
> functions that are not followed by a test with IS_ERR.
> I can post these other scripts if wanted.
>
>
>
> Any ideas to improve or speed-up the coccinelle scripts are welcome.
> Julia ?
>
>
> Best regards,
> CJ
>
>
>
> Coccinelle script n°1:
> =====================
> @find@
> identifier f;
> @@
>
>    f(...)
>    {
>       ...
>       return NULL;
>    }
>
> @script:python@
> f << find.f;
> @@
>
> print "%s" %(f)
>
>
>
> @find2@
> identifier f;
> expression x;
> statement S;
> @@
>
>    x = f(...);
> (
>    if (x) S
> |
>    if (!x) S
> )
>
> @script:python@
> f << find2.f;
> @@
>
> print "%s" %(f)
>
>
>
>
>
> Coccinelle script n°2:
> =====================
> @find@
> statement S;
> type t;
> t *x;
> identifier f;
> @@
>
>     x = f(...);
> (
>     if (IS_ERR(x)) S
> |
>     if (!IS_ERR(x)) S
> )
>
>
>
> @script:python@
> f << find.f;
> @@
>
> print "%s" %(f)

Shouldn't there be a rule like the return NULL here too?

You may also want to take into account the possibility of

ret = NULL;
... when != ret = e
return ret;

julia