Re: [PATCH 03/16] xfs: rationalize dir2_sf entry condition asserts

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Apr 30, 2024 at 02:49:13PM +0200, Christoph Hellwig wrote:
> Various routines dealing with shortform directories have a similar
> pre-condition ASSERT boilerplate that does look a bit weird.
> 
> Remove the assert that the inode fork is non-NULL as it doesn't buy
> anything over the NULL pointer dereference if it is.
> 
> Remove the duplicate i_disk_size ASSERT that uses the less precise
> location of the parent inode number over the one using
> xfs_dir2_sf_hdr_size().
> 
> Remove the if_nextents assert in xfs_dir2_sf_to_block as that is implied
> by the local formt (and not checked by the other functions either).

               format

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/libxfs/xfs_dir2_block.c |  4 ----
>  fs/xfs/libxfs/xfs_dir2_sf.c    | 12 ++----------
>  2 files changed, 2 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
> index 0f93ed1a4a74f4..035a54dbdd7586 100644
> --- a/fs/xfs/libxfs/xfs_dir2_block.c
> +++ b/fs/xfs/libxfs/xfs_dir2_block.c
> @@ -1105,12 +1105,8 @@ xfs_dir2_sf_to_block(
>  	trace_xfs_dir2_sf_to_block(args);
>  
>  	ASSERT(ifp->if_format == XFS_DINODE_FMT_LOCAL);
> -	ASSERT(dp->i_disk_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
> -
>  	ASSERT(ifp->if_bytes == dp->i_disk_size);
> -	ASSERT(oldsfp != NULL);
>  	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(oldsfp->i8count));
> -	ASSERT(dp->i_df.if_nextents == 0);
>  
>  	/*
>  	 * Copy the directory into a temporary buffer.
> diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> index 17a20384c8b719..1cd5228e1ce6af 100644
> --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> @@ -378,9 +378,7 @@ xfs_dir2_sf_addname(
>  
>  	ASSERT(xfs_dir2_sf_lookup(args) == -ENOENT);
>  	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
> -	ASSERT(dp->i_disk_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  	ASSERT(dp->i_df.if_bytes == dp->i_disk_size);
> -	ASSERT(sfp != NULL);
>  	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
>  	/*
>  	 * Compute entry (and change in) size.
> @@ -855,9 +853,7 @@ xfs_dir2_sf_lookup(
>  	xfs_dir2_sf_check(args);
>  
>  	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
> -	ASSERT(dp->i_disk_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  	ASSERT(dp->i_df.if_bytes == dp->i_disk_size);
> -	ASSERT(sfp != NULL);
>  	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
>  	/*
>  	 * Special case for .
> @@ -920,21 +916,19 @@ xfs_dir2_sf_removename(
>  	struct xfs_inode	*dp = args->dp;
>  	struct xfs_mount	*mp = dp->i_mount;
>  	struct xfs_dir2_sf_hdr	*sfp = dp->i_df.if_data;
> +	int			oldsize = dp->i_disk_size;
>  	int			byteoff;	/* offset of removed entry */
>  	int			entsize;	/* this entry's size */
>  	int			i;		/* shortform entry index */
>  	int			newsize;	/* new inode size */
> -	int			oldsize;	/* old inode size */
>  	xfs_dir2_sf_entry_t	*sfep;		/* shortform directory entry */
>  
>  	trace_xfs_dir2_sf_removename(args);
>  
>  	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
> -	oldsize = (int)dp->i_disk_size;
> -	ASSERT(oldsize >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  	ASSERT(dp->i_df.if_bytes == oldsize);
> -	ASSERT(sfp != NULL);
>  	ASSERT(oldsize >= xfs_dir2_sf_hdr_size(sfp->i8count));
> +
>  	/*
>  	 * Loop over the old directory entries.
>  	 * Find the one we're deleting.
> @@ -1028,9 +1022,7 @@ xfs_dir2_sf_replace(
>  	trace_xfs_dir2_sf_replace(args);
>  
>  	ASSERT(dp->i_df.if_format == XFS_DINODE_FMT_LOCAL);
> -	ASSERT(dp->i_disk_size >= offsetof(struct xfs_dir2_sf_hdr, parent));
>  	ASSERT(dp->i_df.if_bytes == dp->i_disk_size);
> -	ASSERT(sfp != NULL);
>  	ASSERT(dp->i_disk_size >= xfs_dir2_sf_hdr_size(sfp->i8count));
>  
>  	/*
> -- 
> 2.39.2
> 
>