Re: [Outreachy kernel] [PATCH 1/2] Staging: lustre: ptlrpc: Make functions static

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 01 October 2015 15:20:39 Julia Lawall wrote:
> On Thu, 1 Oct 2015, Shraddha Barke wrote:
> 
> >
> >
> > On Thu, 1 Oct 2015, Arnd Bergmann wrote:
> >
> > > On Thursday 01 October 2015 15:07:57 Shraddha Barke wrote:
> > > > Since these functions are not called anywhere apart from this particular
> > > > file, declare them as static.
> > > >
> > > > Signed-off-by: Shraddha Barke <shraddha.6596@gmail.com>
> > > >
> > >
> > > Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> > >
> > > Note that there are hundreds more symbols in lustre that should be made
> > > static.
> > > Most of them are harder to find because they have a declaration in a header
> > > file
> > > despite being only used in one place.
> 
> Why does it make it hard to find?  One can make a spearate check in .c
> files.

We currently get a sparse warning for each global identifier definition
that is not preceded by a declaration, but since most of our tools
(gcc, sparse, coccinelle, ...) work on a single file at a time, they
do not know whether a global symbol that is defined in a C file and
(incorrectly) declared in a header is used in another C file or not.

In case of lustre, a lot of functions were originally global because they
were used in the server-side code. However, the code we have in staging
today only includes the client-side implementation and we want to
remove all traces of the server side from it for simplicity.

> I would think though that the clean up could be a little bit tedious.  I
> guess that when the function is static, it shouldn't be declared in the
> header file any more?

Correct. The first step could be to remove declarations for the symbols,
followed by actually removing the ones that we then get sparse warnings
for.

>  At least not if the header file is shared
> between multipe files.  But it still may need to be declared somewhere, if
> it is referenced before it is declared in the one file where it is used.

I would also consider that a bug we should warn about: basically any
'extern' variable declaration and any forward declaration for a 
global function (with or without the redundant extern specifier) should
be considered a bug. We want the same declaration to be visible by both
the file that defines it and the file that uses it so we get warned about
type mismatches.

> The question then is how to divide up the patches.  Should one just
> address all of the functions that have this property in a single file?  Or
> should one only work on a couple of functions at a time that seem related
> to each other?

I think either way would be fine.

There is one more thing to look out for: if a symbol is only used in
the file in which it gets defined, but the use comes before the
definition, we still need a 'static' forward declaration. Ideally
the functions should be reordered in a way that avoids those declarations,
but we don't want patches that mix a large scale move of code around
a file with a simpler change to make the symbols static.

The two parts can be done in either order: we could mark the functions
static and add the forward declarations in one patch, and move them
to their proper place in a follow-up, or do the reverse without the
forward declaration.

> Another issue in Lustre is that there are a some EXPORT_SYMBOLs on
> functyions that are only used in one file.  Should they all be removed, or
> is it like the case of android where there is code not yet in the kernel
> that may rely on them?

They should all be removed as far as I can tell. It's a bug to export
a static symbol. I don't think that happens a lot, but this is something
we could easily check for using coccinelle.

	Arnd