[PATCH] xfs: silence a cppcheck warning

[PATCH] xfs: silence a cppcheck warning

[Gao Xiang]

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;
 	}
-- 
2.27.0

Re: [PATCH] xfs: silence a cppcheck warning

[Dave Chinner]

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>

Title of the patch needs to indicate the fix being made, not
the tool that reported the issue.

So:

xfs: fix logic in sb_unit alignment checks

The cppcheck static analysis checker reported this 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)) {

Modify the logic to avoid the warning.

Fixes: xxxx ("yyyy")
SOB:....

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: silence a cppcheck warning

[Gao Xiang]

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
> 

Re: [PATCH] xfs: silence a cppcheck warning

[Dave Chinner]

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