On Wed, Jul 15, 2020 at 07:38:49PM +0100, Christoph Hellwig wrote: > On Tue, Jul 14, 2020 at 02:46:43PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > > > The onstack inode in xfs_check's process_inode is a potential landmine > > since it's not a /real/ incore inode. The upcoming 5.8 merge will make > > this messier wrt inode forks, so just remove the onstack inode and > > reference the ondisk fields directly. This also reduces the amount of > > thinking that I have to do w.r.t. future libxfs porting efforts. > > > > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> > > Comparing this to my version here: > > http://git.infradead.org/users/hch/xfsprogs.git/commitdiff/791d7d324290dbb83be7bf35fe15f9898f5df1c1 > > > mode_t mode; > > + uint16_t diflags; > > + uint64_t diflags2 = 0; <shrug> I'd rather just leave these over cluttering up the function with repeated flags/flags2 endian conversions. > > + xfs_nlink_t nlink; Actually, we do need this one for proper error reporting because we still (sort of) have to have v1 disk format support. For that matter, the nlink checking further in that function looks totally wrong; it should be using nlink and not accessing the raw field directly. > > + xfs_dqid_t uid; > > + xfs_dqid_t gid; > > + xfs_dqid_t prid; These actually are needed, because the quota_add function that uses them requires pointers to xfs_dqid_t to do some hazy magic to skip quota accounting if the pointer is null, which implies local variables since the ondisk inode has be32 values... That whole thing is messy (and gets worse with the project id) but I'd rather not go rearchitecting more of db/check.c seeing as we can probably just kill it after xfsprogs 5.8 releases. > Not sure we really need the local variables, as they are mostly just > used once except for error messages.. > > > + if (dip->di_version == 1) { > > + nlink = be16_to_cpu(dip->di_onlink); > > + prid = 0; > > + } else { > > + nlink = be32_to_cpu(dip->di_nlink); > > + prid = (xfs_dqid_t)be16_to_cpu(dip->di_projid_hi) << 16 | > > + be16_to_cpu(dip->di_projid_lo); > > + } > > I mad the assumption that we don't support v1 inodes anymore, but > it appears we actually do. So we might need to keep these two. <nod> > > if (isfree) { > > - if (xino.i_d.di_nblocks != 0) { > > + if (be64_to_cpu(dip->di_nblocks) != 0) { > > No need to byte swap for a comparism with 0. > > > - if ((unsigned int)xino.i_d.di_aformat > XFS_DINODE_FMT_BTREE) { > > + if ((unsigned int)dip->di_aformat > XFS_DINODE_FMT_BTREE) { > > No need for the (pre-existing) cast here. > > > + fmtnames[(int)dip->di_aformat], Fixed all of these. --D > > Same here.
