Re: [PATCH] UBIFS: Remove some dead code

[Julia Lawall]

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@xxxxxxxxxx>
> > > ---
> > >   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