Re: [xfs:for-next 70/70] fs/xfs/xfs_da_btree.c:153:26: sparse: symbol 'xfs_da_node_buf_ops' was not declared. Should it be static?

[Dave Chinner <david@fromorbit.com>]

On Mon, Nov 19, 2012 at 10:56:32AM +0800, Fengguang Wu wrote:
> Hi Dave,
> 
> > > + fs/xfs/xfs_dquot.c:294:1: sparse: symbol 'xfs_dquot_buf_write_verify' was not declared. Should it be static?
> > > 
> > > Please consider folding the attached diff :-)
> > 
> > No, for the same reason as the last one. Though I'll fix the new
> > ones (the read/write verifier functions) as they should now be
> > static as a separate patch.
> 
> OK, thanks.
> 
> > > diff --git a/fs/xfs/xfs_bmap.c b/fs/xfs/xfs_bmap.c
> > > index 0e92d12..3216738 100644
> > > --- a/fs/xfs/xfs_bmap.c
> > > +++ b/fs/xfs/xfs_bmap.c
> > > @@ -4180,7 +4180,7 @@ error0:
> > >  /*
> > >   * Add bmap trace insert entries for all the contents of the extent records.
> > >   */
> > > -void
> > > +static void
> > >  xfs_bmap_trace_exlist(
> > >  	xfs_inode_t	*ip,		/* incore inode pointer */
> > >  	xfs_extnum_t	cnt,		/* count of entries in the list */
> > 
> > And, again, there are lots of changes in this that are unrelated to
> > the patch.  In this case, the change is plain wrong. It's a debug
> > only function, called via the macro XFS_BMAP_TRACE_EXLIST:
> > 
> > $ git grep XFS_BMAP_TRACE_EXLIST
> > fs/xfs/xfs_bmap.c:      XFS_BMAP_TRACE_EXLIST(ip, i, whichfork);
> > fs/xfs/xfs_bmap.h:#define       XFS_BMAP_TRACE_EXLIST(ip,c,w)   \
> > fs/xfs/xfs_bmap.h:#define       XFS_BMAP_TRACE_EXLIST(ip,c,w)
> > fs/xfs/xfs_inode.c:             XFS_BMAP_TRACE_EXLIST(ip, nex, whichfork);
> > fs/xfs/xfs_inode.c:     XFS_BMAP_TRACE_EXLIST(ip, nrecs, whichfork);
> > 
> > And so it clearly needs to be non-static.
> 
> Ah OK, that macro does confuse sparse..

It shouldn't. You've clearly got sparse reporting on stuff that is
surrounded by #ifdef DEBUG guards, and that should not be happening.

I get this:

$ make -j8 C=1 fs/xfs/xfs.ko 2>&1 |grep static
fs/xfs/xfs_dir2_leaf.c:82:1: warning: symbol 'xfs_dir2_leafn_read_verify' was not declared. Should it be static?
fs/xfs/xfs_dir2_leaf.c:89:1: warning: symbol 'xfs_dir2_leafn_write_verify' was not declared. Should it be static?
fs/xfs/xfs_dquot.c:339:1: warning: symbol 'xfs_dquot_buf_write_verify' was not declared. Should it be static?
$

And there is no warnings about anything inside DEBUG builds. So you
must be running the tool with some strange set of options, or you
are running it with CONFIG_XFS_DEBUG=y. But you can't be doing that,
either, because:

$ make -j8 C=1 fs/xfs/xfs.ko 2>&1 |grep static | wc -l
283
$ make -j8 C=1 fs/xfs/xfs.ko 2>&1 |grep static | grep exlist
$

sparse is not issuing warnings about xfs_bmap_trace_exlist() needing
to be static on CONFIG_XFS_DEBUG=y builds.

So the build bot is doing something strange and unusual, and getting
false warnings as a result...

> > If you are going throw commit-by-commit build warnings and patches
> > to fix them, please only include the fixes for the *new* warnings
> > generated by a single commit, not an aggregate of everything that is
> > found. 
> 
> Fair enough. However I'd like to do it in a slightly different way.
> 
> The problem is that there are lots of existing (ie. old) valid
> warnings on the missing "static". I'd still like the auto generated
> patch to fix these old ones by the way.

Sure, but don't mix them with fixes for new warnings. And if they
are NAKed, then never send them again ;)

> At the same time, to avoid the
> *duplicated* chunks, I'll tell the script to remember the list of
> symbols that have been made static by the generated patches. This
> should address your concern, while still be able to gradually get rid
> of the existing static warnings.

Sure.

> 
> > For that reason, I think I'd prefer it if your build bot
> > just sent build warnings, not patches.
> 
> I think the patches could be improved rather than removed. I'll fix
> the duplicated patches like in this case.
> 
> Since there tend to be lots of "Should it be static?" warnings, it
> would save some (boring) human time by providing an auto generated
> patch for consideration.

>From my perspective, it takes longer to validate that the warning is
correct (espcially given these cases where the warning is clearly
wrong and indicates a problem with the bot) and then that the patch
is correct as it does to find and fix these problems myself.

And, of course, the only reason I missed these is that my last set
of checks on these patches were on a debug build and I was looking
for endian problems so I filtered out all the static warnings...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs