On Fri, Aug 13, 2021 at 07:40:48AM +1000, Dave Chinner wrote:

On Thu, Aug 12, 2021 at 09:42:22AM +0300, Dan Carpenter wrote:

quoted

Hello Darrick J. Wong,

The patch c809d7e948a1: "xfs: pass the goal of the incore inode walk
to xfs_inode_walk()" from Jun 1, 2021, leads to the following
Smatch static checker warning:

	fs/xfs/xfs_icache.c:52 xfs_icwalk_tag()
	warn: unsigned 'goal' is never less than zero.

fs/xfs/xfs_icache.c
    49 static inline unsigned int
    50 xfs_icwalk_tag(enum xfs_icwalk_goal goal)
    51 {
--> 52 	return goal < 0 ? XFS_ICWALK_NULL_TAG : goal;

This enum will be unsigned in GCC, so "goal" can't be negative.

I think this is incorrect. The original C standard defines enums as
signed integers, not unsigned. And according to the GCC manual
(section 4.9 Structures, Unions, Enumerations, and Bit-Fields)
indicates that C90 first defines the enum type to be compatible with
the declared values. IOWs, for a build using C89 like the kernel
does, enums should always be signed.

This enum is defined as:

enum xfs_icwalk_goal {
        /* Goals that are not related to tags; these must be < 0. */
        XFS_ICWALK_DQRELE       = -1,

        /* Goals directly associated with tagged inodes. */
        XFS_ICWALK_BLOCKGC      = XFS_ICI_BLOCKGC_TAG,
        XFS_ICWALK_RECLAIM      = XFS_ICI_RECLAIM_TAG,
};

i.e. the enum is defined to clearly contain negative values and so
GCC should be defining it as a signed integer regardless of the
version of C being used...

You're analysis is correct, but I'm looking at a newer version of the
code and I blamed the wrong commit.  It should be commit 777eb1fa857e
("xfs: remove xfs_dqrele_all_inodes")
https://lore.kernel.org/linux-xfs/20210809065938.1199181-3-hch@lst.de/ (local)
That commit removes the "XFS_ICWALK_DQRELE       = -1," line which
changes the enum type from int to unsigned int.

So this suggests that we should just remove the check for negative
values.

regards,
dan carpenter

quoted

Plus
we only pass 0-1 for goal (as far as Smatch can tell).

Yup, smatch has definitely got that one wrong:

xfs_dqrele_all_inodes()
  xfs_icwalk(mp, XFS_ICWALK_DQRELE, &icw);
    xfs_icwalk_get_perag(.... XFS_ICWALK_DQRELE)
      xfs_icwalk_tag(... XFS_ICWALK_DQRELE, ...)

So this warning looks like an issue with smatch, not a bug in the
code...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com