Re: [PATCH] fgraph: fix unused value in register_ftrace_graph()

[Gianfranco Trad]

--- Redirecting locally, forgot to cc mailing list ---

On 22/10/24 08:56, Steven Rostedt wrote:
> On Mon, 21 Oct 2024 12:24:29 +0200
> Gianfranco Trad <gianf.trad@xxxxxxxxx> wrote:
>
> > Coverity reports unused assignment to value ret. [1]
> > ret is assigned to 0 here, but that stored value is overwritten before
> > it can be used. The overwrite might happen either at line 1277, 1290
> > or eventually at line 1306. Therefore, cleanup the unused assignment.
> >
> > [1] Coverity Scan, CID 1633338
>
> What does the above mean? For such a simple change, is it really
> unnecessary?

Static analyzer complains that ret is assigned again to 0 there (doesn't 
complain about the declaration phase in which it was already assigned to 
0) but no instructions later make use of this re-assigned value.
Therefore, it flags it as not needed assignment.

> > Signed-off-by: Gianfranco Trad <gianf.trad@xxxxxxxxx>
> > ---
> >   kernel/trace/fgraph.c | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> > index 41e7a15dcb50..cc2e065c1c8d 100644
> > --- a/kernel/trace/fgraph.c
> > +++ b/kernel/trace/fgraph.c
> > @@ -1262,7 +1262,6 @@ int register_ftrace_graph(struct fgraph_ops *gops)
> >   			return ret;
> >   		}
> >   		fgraph_initialized = true;
> > -		ret = 0;
>
> Not to mention that if this is to go, why not get rid of the
> declaration part too? That is:
>
> -	int ret = 0;
> +	int ret;

I see this, but the static analyzer will actually complain if we get rid 
of int ret = 0 in the declaration phase here, saying something in the 
lines "ret uninitialized at declaration".
To recap, Coverity complains that after declaration ret is assigned 
again to 0 (line 1262) without using this specific re-assignment because 
later on instructions overwrite this value, before it is even used by 
any instruction.

> But still, this code is about to be merged with other code where this
> change may cause issues. This is such a slow path that getting rid of
> the extra initialization may not be worth it.

Understood. This report by Coverity can be considered something trivial, 
so it's up to you what to do. Just wanted to report it and help.

Have a nice day,

-- Gian

> -- Steve
>
>
> >   	}
> >   
> >   	if (!fgraph_array[0]) {