Re: [PATCH] xfs: silence a cppcheck warning

[Dave Chinner <david@fromorbit.com>]

On Fri, Dec 11, 2020 at 10:09:44AM +0800, Gao Xiang wrote:
> Hi Dave,
> 
> On Fri, Dec 11, 2020 at 12:17:44PM +1100, Dave Chinner wrote:
> > On Fri, Dec 11, 2020 at 07:57:47AM +0800, Gao Xiang wrote:
> > > This patch silences a new cppcheck static analysis warning
> > > >> fs/xfs/libxfs/xfs_sb.c:367:21: warning: Boolean result is used in bitwise operation. Clarify expression with parentheses. [clarifyCondition]
> > >     if (!!sbp->sb_unit ^ xfs_sb_version_hasdalign(sbp)) {
> > > 
> > > introduced from my patch. Sorry I didn't test it with cppcheck before.
> > > 
> > > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > 
> 
> ...
> 
> > > ---
> > >  fs/xfs/libxfs/xfs_sb.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > index bbda117e5d85..ae5df66c2fa0 100644
> > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > @@ -360,11 +360,8 @@ xfs_validate_sb_common(
> > >  		}
> > >  	}
> > >  
> > > -	/*
> > > -	 * Either (sb_unit and !hasdalign) or (!sb_unit and hasdalign)
> > > -	 * would imply the image is corrupted.
> > > -	 */
> > > -	if (!!sbp->sb_unit ^ xfs_sb_version_hasdalign(sbp)) {
> > > +	if ((sbp->sb_unit && !xfs_sb_version_hasdalign(sbp)) ||
> > > +	    (!sbp->sb_unit && xfs_sb_version_hasdalign(sbp))) {
> > >  		xfs_notice(mp, "SB stripe alignment sanity check failed");
> > >  		return -EFSCORRUPTED;
> > 
> > But, ummm, what's the bug here? THe logic looks correct to me -
> > !!sbp->sb_unit will have a value of 0 or 1, and
> > xfs_sb_version_hasdalign() returns a bool so will also have a value
> > of 0 or 1. That means the bitwise XOR does exactly the correct thing
> > here as we are operating on two boolean values. So I don't see a bug
> > here, nor that it's a particularly useful warning.
> > 
> > FWIW, I've never heard of this "cppcheck" analysis tool. Certainly
> > I've never used it, and this warning seems to be somewhat
> > questionable so I'm wondering if this is just a new source of random
> > code churn or whether it's actually finding real bugs?
> 
> Here is a reference of the original report:
> https://www.mail-archive.com/kbuild@lists.01.org/msg05057.html

Ok, so it's just generating noise, not pointing out actual bugs.
Yup:

cppcheck possible warnings: (new ones prefixed by >>, may not real problems)

So it's even telling us that it might just be generating noise.

> The reason I didn't add "Fixes:" or "Reported-by:" or use "fix" in the
> subject since I (personally) don't think it's worth adding, since I
> have no idea when linux kernel runs with "cppcheck" analysis tool
> (I only heard "sparse and smatch are using "before.) and I don't think
> it's actually a bug here.
> 
> If "cppcheck" should be considered, I'm also wondering what kind of
> options should be used for linux kernel. And honestly, there are many
> other analysis tools on the market, many of them even complain about
> "strcpy" and should use "strcpy_s" instead (or many other likewise).
> 
> Personally I don't think it's even worth adding some comments about
> this since it's a pretty straight-forward boolean algebra on my side
> (but yeah, if people don't like it, I can update it as well since
>  it's quite minor to me.)

If the checker is not pointing out actual bugs, we should just
ignore it. That's what we do with coverity, etc. The code is fine,
I don't find it hard to read or in any way confusing, so I think
it's fine to ignore it...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com