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@xxxxxxxxxx>
> >
>
> ...
>
> > > ---
> > > 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@xxxxxxxxxxxx/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@xxxxxxxxxxxxx
