Re: [PATCH] xfs: silence a cppcheck warning

[Gao Xiang <hsiangkao@redhat.com>]

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

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.)

Thanks,
Gao Xiang

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>