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 <fengguang.wu@intel.com>]

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