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@xxxxxxxxxxxxx _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
