[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?

[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]

[-- 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

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]

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

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?

[Fengguang Wu]

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

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]

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

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?

[Fengguang Wu]

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