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

[Christoph Hellwig <hch@infradead.org>]

>  	bp = xfs_btree_get_bufs(tp->t_mountp, tp, agno, agbno);
> -	if (!bp) {
> -		XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, tp->t_mountp);
> +	if (XFS_IS_CORRUPT(tp->t_mountp, !bp)) {
>  		return -EFSCORRUPTED;
>  	}

We can kill the braces here now.  Same for various other spots later
down.

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

Somewhat strange indentation here.

>  	ASSERT(map && *map);
> @@ -2566,14 +2551,16 @@ xfs_dabuf_map(
>  		nirecs = 1;
>  	}
>  
> -	if (!xfs_da_map_covers_blocks(nirecs, irecs, bno, nfsb)) {
> -		/* Caller ok with no mapping. */
> -		if (mappedbno == -2) {
> -			error = -1;
> -			goto out;
> -		}
> +	covers_blocks = xfs_da_map_covers_blocks(nirecs, irecs, bno, nfsb);
> +
> +	/* Caller ok with no mapping. */
> +	if (mappedbno == -2 && !covers_blocks) {
> +		error = -1;
> +		goto out;
> +	}
>  
> -		/* Caller expected a mapping, so abort. */
> +	/* Caller expected a mapping, so abort. */
> +	if (XFS_IS_CORRUPT(mp, !covers_blocks)) {

Why the restructure here?

This could have just become:

		if (!XFS_IS_CORRUPT(mp != -2)) {
			error = -1;
			goto out;
		}

not that I really like the current structure, but that change seems bit
out of place in these semi-mechanical fixups, and once we touch the
structure of this function and its callers there is so much more to
fix..

> index 7b845c052fb4..e1b9de6c7437 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -87,6 +87,10 @@ struct xfs_ifork {
>  #define XFS_IFORK_MAXEXT(ip, w) \
>  	(XFS_IFORK_SIZE(ip, w) / sizeof(xfs_bmbt_rec_t))
>  
> +#define XFS_IFORK_MAPS_BLOCKS(ip, w) \
> +		(XFS_IFORK_FORMAT((ip), (w)) == XFS_DINODE_FMT_EXTENTS || \
> +		 XFS_IFORK_FORMAT((ip), (w)) == XFS_DINODE_FMT_BTREE)

Why the double indentation?  Also maybe XFS_IFORK_FORMAT_MAPS_BLOCKS
is a better name?  Or maybe even turn it into an inline function with
a less shouting name?  Also the addition of this helper is probably
worth being split into a separate patch.

> +		    head_block >= tail_block || head_cycle != (tail_cycle + 1)))

no need for the inner most braces here if you touch the line anyway.