| Message ID | 20201210235747.469708-1-hsiangkao@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfs: silence a cppcheck warning | expand |
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.
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 >
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.
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; }