Re: [PATCH 2/2] xfs: convert open coded corruption check to use XFS_IS_CORRUPT

[Dave Chinner <david@fromorbit.com>]

On Thu, Nov 07, 2019 at 11:05:21PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Convert the last of the open coded corruption check and report idioms to
> use the XFS_IS_CORRUPT macro.

hmmm.

> +	if (XFS_IS_CORRUPT(mp,
> +	    ir.loaded != XFS_IFORK_NEXTENTS(ip, whichfork))) {

This pattern is weird. It looks like there are two separate logic
statements to the if() condition, when in fact the second line is
part of the XFS_IS_CORRUPT() macro.

It just looks wrong to me, especially when everything other
multi-line macro is indented based on the indenting of the macro
parameters....

Yes, in this case it looks a bit strange, too:

	if (XFS_IS_CORRUPT(mp,
			   ir.loaded != XFS_IFORK_NEXTENTS(ip, whichfork))) {

but there is no mistaking it for separate logic statements.

I kinda value being able to glance at the indent levels to see
separate logic elements....

> -		if (unlikely(
> -		       be32_to_cpu(sib_info->back) != last_blkno ||
> -		       sib_info->magic != dead_info->magic)) {
> -			XFS_ERROR_REPORT("xfs_da_swap_lastblock(3)",
> -					 XFS_ERRLEVEL_LOW, mp);
> +		if (XFS_IS_CORRUPT(mp,
> +		    be32_to_cpu(sib_info->back) != last_blkno ||
> +		    sib_info->magic != dead_info->magic)) {
>  			error = -EFSCORRUPTED;
>  			goto done;
>  		}

This is kind of what I mean - is it two or three  logic statments
here? No, it's actually one, but it has two nested checks...

There's a few other list this that are somewhat non-obvious as to
the logic...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com