Re: [PATCH v2] xfs: rearrange the logic and remove the broken comment for xfs_dir2_isxx

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

On Sun, Sep 18, 2022 at 02:50:26PM +0800, Stephen Zhang wrote:
> xfs_dir2_isleaf is used to see if the directory is a single-leaf
> form directory instead, as commented right above the function.
> 
> Besides getting rid of the broken comment, we rearrange the logic by
> converting everything over to standard formatting and conventions,
> at the same time, to make it easier to understand and self documenting.
> 
> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
> ---
> Changes from v1:
> - v1 is only designed to fix the broken comment, while v2 rearranges the
>   logic in addition to that, which is suggested by Dave.
> ---
>  fs/xfs/libxfs/xfs_dir2.c  | 50 +++++++++++++++++++++++----------------
>  fs/xfs/libxfs/xfs_dir2.h  |  4 ++--
>  fs/xfs/scrub/dir.c        |  2 +-
>  fs/xfs/xfs_dir2_readdir.c |  2 +-
>  4 files changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index 76eedc2756b3..33738165d67d 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -261,7 +261,7 @@ xfs_dir_createname(
>  {
>  	struct xfs_da_args	*args;
>  	int			rval;
> -	int			v;		/* type-checking value */
> +	bool			v;		/* type-checking value */
>  
>  	ASSERT(S_ISDIR(VFS_I(dp)->i_mode));
>  
> @@ -357,7 +357,7 @@ xfs_dir_lookup(
>  {
>  	struct xfs_da_args	*args;
>  	int			rval;
> -	int			v;	  /* type-checking value */
> +	bool			v;	  /* type-checking value */
>  	int			lock_mode;
>  
>  	ASSERT(S_ISDIR(VFS_I(dp)->i_mode));
> @@ -435,7 +435,7 @@ xfs_dir_removename(
>  {
>  	struct xfs_da_args	*args;
>  	int			rval;
> -	int			v;		/* type-checking value */
> +	bool			v;		/* type-checking value */
>  
>  	ASSERT(S_ISDIR(VFS_I(dp)->i_mode));
>  	XFS_STATS_INC(dp->i_mount, xs_dir_remove);
> @@ -493,7 +493,7 @@ xfs_dir_replace(
>  {
>  	struct xfs_da_args	*args;
>  	int			rval;
> -	int			v;		/* type-checking value */
> +	bool			v;		/* type-checking value */
>  
>  	ASSERT(S_ISDIR(VFS_I(dp)->i_mode));
>  
> @@ -610,19 +610,23 @@ xfs_dir2_grow_inode(
>  int
>  xfs_dir2_isblock(
>  	struct xfs_da_args	*args,
> -	int			*vp)	/* out: 1 is block, 0 is not block */
> +	bool			*isblock)
>  {
> -	xfs_fileoff_t		last;	/* last file offset */
> -	int			rval;
> +	struct xfs_mount	*mp = args->dp->i_mount;
> +	xfs_fileoff_t		eof;
> +	int			error;
>  
> -	if ((rval = xfs_bmap_last_offset(args->dp, &last, XFS_DATA_FORK)))
> -		return rval;
> -	rval = XFS_FSB_TO_B(args->dp->i_mount, last) == args->geo->blksize;
> -	if (XFS_IS_CORRUPT(args->dp->i_mount,
> -			   rval != 0 &&
> -			   args->dp->i_disk_size != args->geo->blksize))
> +	error = xfs_bmap_last_offset(args->dp, &eof, XFS_DATA_FORK);
> +	if (error)
> +		return error;
> +
> +	*isblock = false;
> +	if (XFS_FSB_TO_B(mp, eof) != args->geo->blksize)
> +		return 0;
> +
> +	*isblock = true;
> +	if (XFS_IS_CORRUPT(mp, args->dp->i_disk_size != args->geo->blksize))
>  		return -EFSCORRUPTED;
> -	*vp = rval;
>  	return 0;

Stylistic note: One has to be careful with these functions that pass out
a value /and/ potentially return a negative errno -- one has to be
careful about specifying whether or not callers should expect the out
parameter to be set if an errno is returned.  I think it looks slightly
better to see things like:

	if (XFS_FSB_TO_B(mp, eof) != args->geo->blksize) {
		*isblock = false;
		return 0;
	}

	if (XFS_IS_CORRUPT(mp, args->dp->i_disk_size != args->geo->blksize))
		return -EFSCORRUPTED;

	*isblock = true;
	return 0;


But for this patch that really doesn't matter, so:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


>  }
>  
> @@ -632,14 +636,20 @@ xfs_dir2_isblock(
>  int
>  xfs_dir2_isleaf(
>  	struct xfs_da_args	*args,
> -	int			*vp)	/* out: 1 is block, 0 is not block */
> +	bool			*isleaf)
>  {
> -	xfs_fileoff_t		last;	/* last file offset */
> -	int			rval;
> +	xfs_fileoff_t		eof;
> +	int			error;
>  
> -	if ((rval = xfs_bmap_last_offset(args->dp, &last, XFS_DATA_FORK)))
> -		return rval;
> -	*vp = last == args->geo->leafblk + args->geo->fsbcount;
> +	error = xfs_bmap_last_offset(args->dp, &eof, XFS_DATA_FORK);
> +	if (error)
> +		return error;
> +
> +	*isleaf = false;
> +	if (eof != args->geo->leafblk + args->geo->fsbcount)
> +		return 0;
> +
> +	*isleaf = true;
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> index b6df3c34b26a..dd39f17dd9a9 100644
> --- a/fs/xfs/libxfs/xfs_dir2.h
> +++ b/fs/xfs/libxfs/xfs_dir2.h
> @@ -61,8 +61,8 @@ extern int xfs_dir2_sf_to_block(struct xfs_da_args *args);
>  /*
>   * Interface routines used by userspace utilities
>   */
> -extern int xfs_dir2_isblock(struct xfs_da_args *args, int *r);
> -extern int xfs_dir2_isleaf(struct xfs_da_args *args, int *r);
> +extern int xfs_dir2_isblock(struct xfs_da_args *args, bool *isblock);
> +extern int xfs_dir2_isleaf(struct xfs_da_args *args, bool *isleaf);
>  extern int xfs_dir2_shrink_inode(struct xfs_da_args *args, xfs_dir2_db_t db,
>  				struct xfs_buf *bp);
>  
> diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
> index 5abb5fdb71d9..b9c5764e7437 100644
> --- a/fs/xfs/scrub/dir.c
> +++ b/fs/xfs/scrub/dir.c
> @@ -676,7 +676,7 @@ xchk_directory_blocks(
>  	xfs_dablk_t		dabno;
>  	xfs_dir2_db_t		last_data_db = 0;
>  	bool			found;
> -	int			is_block = 0;
> +	bool			is_block = false;
>  	int			error;
>  
>  	/* Ignore local format directories. */
> diff --git a/fs/xfs/xfs_dir2_readdir.c b/fs/xfs/xfs_dir2_readdir.c
> index e295fc8062d8..9f3ceb461515 100644
> --- a/fs/xfs/xfs_dir2_readdir.c
> +++ b/fs/xfs/xfs_dir2_readdir.c
> @@ -512,7 +512,7 @@ xfs_readdir(
>  {
>  	struct xfs_da_args	args = { NULL };
>  	unsigned int		lock_mode;
> -	int			isblock;
> +	bool			isblock;
>  	int			error;
>  
>  	trace_xfs_readdir(dp);
> -- 
> 2.27.0
>