Re: [PATCH 1/3] xfs_db: stop misusing an onstack inode

["Darrick J. Wong" <darrick.wong@oracle.com>]

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@oracle.com>
> > 
> > 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@oracle.com>
> 
> 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.