On Mon, Nov 19, 2012 at 02:38:05PM +1100, Dave Chinner wrote: > 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, Yes I can find CONFIG_XFS_DEBUG=y in my .config. Now I understand your points about "xfs debug build". I've just disabled CONFIG_XFS_DEBUG for sparse builds, so that the stuffs in #ifdef DEBUG won't trigger the false warnings. > 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... My build commands are make ARCH=i386 allmodconfig make ARCH=i386 C=1 fs/xfs/xfs.ko 2>&1 |grep static fs/xfs/xfs_dquot.c:55:15: sparse: symbol 'xfs_dqerror_target' was not declared. Should it be static? fs/xfs/xfs_dquot.c:56:5: sparse: symbol 'xfs_do_dqerror' was not declared. Should it be static? fs/xfs/xfs_dquot.c:57:5: sparse: symbol 'xfs_dqreq_num' was not declared. Should it be static? fs/xfs/xfs_dquot.c:58:5: sparse: symbol 'xfs_dqerror_mod' was not declared. Should it be static? fs/xfs/xfs_dquot.c:215:1: sparse: symbol 'xfs_qm_init_dquot_blk' was not declared. Should it be static? fs/xfs/xfs_dquot.c:294:1: sparse: symbol 'xfs_dquot_buf_write_verify' was not declared. Should it be static? fs/xfs/xfs_dquot.c:310:1: sparse: symbol 'xfs_qm_dqalloc' was not declared. Should it be static? fs/xfs/xfs_dquot.c:416:1: sparse: symbol 'xfs_qm_dqrepair' was not declared. Should it be static? fs/xfs/xfs_dquot.c:467:1: sparse: symbol 'xfs_qm_dqtobp' was not declared. Should it be static? fs/xfs/xfs_dquot.c:838:1: sparse: symbol 'xfs_qm_dqput_final' was not declared. Should it be static? fs/xfs/xfs_dquot.c:925:1: sparse: symbol 'xfs_qm_dqflush_done' was not declared. Should it be static? (only listing the output for xfs_dquot.c) (almost the same results for ARCH=x86_64) > > > 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. OK.. this will take a bit more coding, but I fully understand your points and will do separated fixes for new/old warnings to avoid the confusion. > And if they are NAKed, then never send them again ;) Whether or not they are NAKed, they'll not be sent again ;) Because the symbols will be remembered as "fixed" (by itself) at patch generation time. > > 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. I'd think the patch offers more context to make a judge.. except that mixing the fixes for old/new problems in one patch will be confusing to the commit author who is only responsible for (and aware of) the new warnings. > 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... Thanks, Fengguang _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
