Re: [PATCH] xfs: silence a cppcheck warning

[Dave Chinner <david@fromorbit.com>]

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