* [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? @ 2012-11-16 7:25 kbuild test robot 2012-11-17 23:50 ` Dave Chinner 0 siblings, 1 reply; 5+ messages in thread From: kbuild test robot @ 2012-11-16 7:25 UTC (permalink / raw) To: Dave Chinner; +Cc: Ben Myers, xfs [-- Attachment #1: Type: text/plain, Size: 1261 bytes --] tree: git://oss.sgi.com/xfs/xfs for-next head: 1813dd64057490e7a0678a885c4fe6d02f78bdc1 commit: 1813dd64057490e7a0678a885c4fe6d02f78bdc1 [70/70] xfs: convert buffer verifiers to an ops structure. sparse warnings: + fs/xfs/xfs_da_btree.c:153:26: sparse: symbol 'xfs_da_node_buf_ops' was not declared. Should it be static? -- + fs/xfs/xfs_dir2_leaf.c:82:1: sparse: symbol 'xfs_dir2_leafn_read_verify' was not declared. Should it be static? + fs/xfs/xfs_dir2_leaf.c:89:1: sparse: symbol 'xfs_dir2_leafn_write_verify' was not declared. Should it be 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:294:1: sparse: symbol 'xfs_dquot_buf_write_verify' was not declared. Should it be static? Please consider folding the attached diff :-) --- 0-DAY kernel build testing backend Open Source Technology Center Fengguang Wu, Yuanhan Liu Intel Corporation [-- Attachment #2: make-it-static-1813dd6.diff --] [-- Type: text/x-diff, Size: 3531 bytes --] 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 */ @@ -4767,7 +4767,7 @@ xfs_bmapi_allocate_worker( * them off to a worker thread so there is lots of stack to use. Otherwise just * call directly to avoid the context switch overhead here. */ -int +static int xfs_bmapi_allocate( struct xfs_bmalloca *args) { diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 26673a0..7dcff98 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -1503,7 +1503,7 @@ restart: spin_unlock(&btp->bt_lru_lock); } -int +static int xfs_buftarg_shrink( struct shrinker *shrink, struct shrink_control *sc) diff --git a/fs/xfs/xfs_da_btree.c b/fs/xfs/xfs_da_btree.c index 4d7696a..adf1482 100644 --- a/fs/xfs/xfs_da_btree.c +++ b/fs/xfs/xfs_da_btree.c @@ -150,7 +150,7 @@ xfs_da_node_read_verify( } } -const struct xfs_buf_ops xfs_da_node_buf_ops = { +static const struct xfs_buf_ops xfs_da_node_buf_ops = { .verify_read = xfs_da_node_read_verify, .verify_write = xfs_da_node_write_verify, }; diff --git a/fs/xfs/xfs_dir2_leaf.c b/fs/xfs/xfs_dir2_leaf.c index 60cd2fa..6789e06 100644 --- a/fs/xfs/xfs_dir2_leaf.c +++ b/fs/xfs/xfs_dir2_leaf.c @@ -78,14 +78,14 @@ xfs_dir2_leaf1_write_verify( xfs_dir2_leaf_verify(bp, cpu_to_be16(XFS_DIR2_LEAF1_MAGIC)); } -void +static void xfs_dir2_leafn_read_verify( struct xfs_buf *bp) { xfs_dir2_leaf_verify(bp, cpu_to_be16(XFS_DIR2_LEAFN_MAGIC)); } -void +static void xfs_dir2_leafn_write_verify( struct xfs_buf *bp) { diff --git a/fs/xfs/xfs_dir2_node.c b/fs/xfs/xfs_dir2_node.c index 5980f9b..025f91f 100644 --- a/fs/xfs/xfs_dir2_node.c +++ b/fs/xfs/xfs_dir2_node.c @@ -330,7 +330,7 @@ xfs_dir2_leafn_add( /* * Check internal consistency of a leafn block. */ -void +static void xfs_dir2_leafn_check( struct xfs_inode *dp, struct xfs_buf *bp) diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c index 9e1bf52..e4ab229 100644 --- a/fs/xfs/xfs_dquot.c +++ b/fs/xfs/xfs_dquot.c @@ -52,10 +52,10 @@ */ #ifdef DEBUG -xfs_buftarg_t *xfs_dqerror_target; -int xfs_do_dqerror; -int xfs_dqreq_num; -int xfs_dqerror_mod = 33; +static xfs_buftarg_t *xfs_dqerror_target; +static int xfs_do_dqerror; +static int xfs_dqreq_num; +static int xfs_dqerror_mod = 33; #endif struct kmem_zone *xfs_qm_dqtrxzone; @@ -290,7 +290,7 @@ xfs_dquot_buf_read_verify( xfs_dquot_buf_verify(bp); } -void +static void xfs_dquot_buf_write_verify( struct xfs_buf *bp) { diff --git a/fs/xfs/xfs_ialloc_btree.c b/fs/xfs/xfs_ialloc_btree.c index bec344b..cf52893 100644 --- a/fs/xfs/xfs_ialloc_btree.c +++ b/fs/xfs/xfs_ialloc_btree.c @@ -182,7 +182,7 @@ xfs_inobt_key_diff( cur->bc_rec.i.ir_startino; } -void +static void xfs_inobt_verify( struct xfs_buf *bp) { diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c index 4fc17d4..7fc6c42 100644 --- a/fs/xfs/xfs_trans_buf.c +++ b/fs/xfs/xfs_trans_buf.c @@ -233,10 +233,10 @@ xfs_trans_getsb(xfs_trans_t *tp, } #ifdef DEBUG -xfs_buftarg_t *xfs_error_target; -int xfs_do_error; -int xfs_req_num; -int xfs_error_mod = 33; +static xfs_buftarg_t *xfs_error_target; +static int xfs_do_error; +static int xfs_req_num; +static int xfs_error_mod = 33; #endif /* [-- Attachment #3: Type: text/plain, Size: 121 bytes --] _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 5+ messages in thread
* 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? 2012-11-16 7:25 [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? kbuild test robot @ 2012-11-17 23:50 ` Dave Chinner 2012-11-19 2:56 ` Fengguang Wu 0 siblings, 1 reply; 5+ messages in thread From: Dave Chinner @ 2012-11-17 23:50 UTC (permalink / raw) To: kbuild test robot; +Cc: Ben Myers, xfs, Dave Chinner On Fri, Nov 16, 2012 at 03:25:36PM +0800, kbuild test robot wrote: > tree: git://oss.sgi.com/xfs/xfs for-next > head: 1813dd64057490e7a0678a885c4fe6d02f78bdc1 > commit: 1813dd64057490e7a0678a885c4fe6d02f78bdc1 [70/70] xfs: convert buffer verifiers to an ops structure. > > > sparse warnings: > > + fs/xfs/xfs_da_btree.c:153:26: sparse: symbol 'xfs_da_node_buf_ops' was not declared. Should it be static? > -- > + fs/xfs/xfs_dir2_leaf.c:82:1: sparse: symbol 'xfs_dir2_leafn_read_verify' was not declared. Should it be static? > + fs/xfs/xfs_dir2_leaf.c:89:1: sparse: symbol 'xfs_dir2_leafn_write_verify' was not declared. Should it be 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: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. > 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. 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. For that reason, I think I'd prefer it if your build bot just sent build warnings, not patches. FWIW, what happens when a problem is fixed by a later patch in the tree in the same push? Do you still throw a mail out to the list? i.e. are you culling spurious warning detections? Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* 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? 2012-11-17 23:50 ` Dave Chinner @ 2012-11-19 2:56 ` Fengguang Wu 2012-11-19 3:38 ` Dave Chinner 0 siblings, 1 reply; 5+ messages in thread From: Fengguang Wu @ 2012-11-19 2:56 UTC (permalink / raw) To: Dave Chinner; +Cc: Ben Myers, xfs 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.. > 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. 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. > 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. > FWIW, what happens when a problem is fixed by a later patch in the > tree in the same push? Do you still throw a mail out to the list? > i.e. are you culling spurious warning detections? Yes, the build script has code to avoid sending reports on interim warnings. It will double check the branch HEAD and if the warnings to be reported have disappeared there, nothing will be reported. However if it's a build *error* fixed in the HEAD, it's a potential problem for bisection, so will still be reported unless it's a known tree that won't do rebase (ie. net/master). Thanks, Fengguang _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
* 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? 2012-11-19 2:56 ` Fengguang Wu @ 2012-11-19 3:38 ` Dave Chinner 2012-11-19 7:21 ` Fengguang Wu 0 siblings, 1 reply; 5+ messages in thread From: Dave Chinner @ 2012-11-19 3:38 UTC (permalink / raw) To: Fengguang Wu; +Cc: Ben Myers, xfs 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* 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? 2012-11-19 3:38 ` Dave Chinner @ 2012-11-19 7:21 ` Fengguang Wu 0 siblings, 0 replies; 5+ messages in thread From: Fengguang Wu @ 2012-11-19 7:21 UTC (permalink / raw) To: Dave Chinner; +Cc: Ben Myers, xfs 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@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-11-19 7:33 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-11-16 7:25 [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? kbuild test robot 2012-11-17 23:50 ` Dave Chinner 2012-11-19 2:56 ` Fengguang Wu 2012-11-19 3:38 ` Dave Chinner 2012-11-19 7:21 ` Fengguang Wu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.