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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
--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
>
