[PATCH] xfs: fix up the comment in xfs_dir2_isleaf

[PATCH] xfs: fix up the comment in xfs_dir2_isleaf

[Stephen Zhang]

xfs_dir2_isleaf is used to see if the directory is a single-leaf
form directory, as commented right above the function.

Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
---
 fs/xfs/libxfs/xfs_dir2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
index 76eedc2756b3..1485f53fecf4 100644
--- a/fs/xfs/libxfs/xfs_dir2.c
+++ b/fs/xfs/libxfs/xfs_dir2.c
@@ -632,7 +632,7 @@ xfs_dir2_isblock(
 int
 xfs_dir2_isleaf(
 	struct xfs_da_args	*args,
-	int			*vp)	/* out: 1 is block, 0 is not block */
+	int			*vp)	/* out: 1 is leaf, 0 is not leaf */
 {
 	xfs_fileoff_t		last;	/* last file offset */
 	int			rval;
-- 
2.25.1

Re: [PATCH] xfs: fix up the comment in xfs_dir2_isleaf

[Dave Chinner]

On Sun, Sep 11, 2022 at 11:31:37AM +0800, Stephen Zhang wrote:
> xfs_dir2_isleaf is used to see if the directory is a single-leaf
> form directory, as commented right above the function.
> 
> Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
> ---
>  fs/xfs/libxfs/xfs_dir2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> index 76eedc2756b3..1485f53fecf4 100644
> --- a/fs/xfs/libxfs/xfs_dir2.c
> +++ b/fs/xfs/libxfs/xfs_dir2.c
> @@ -632,7 +632,7 @@ xfs_dir2_isblock(
>  int
>  xfs_dir2_isleaf(
>  	struct xfs_da_args	*args,
> -	int			*vp)	/* out: 1 is block, 0 is not block */
> +	int			*vp)	/* out: 1 is leaf, 0 is not leaf */
>  {
>  	xfs_fileoff_t		last;	/* last file offset */
>  	int			rval;

If you are going to touch this code to fix a broken comment, then
please clean it up properly.

The "*vp" parameter should be a "bool *isleaf", in which case the
return value is obvious and the comment can be removed. Then the
logic in the function can be cleaned up to be obvious instead of
relying on easy to mistake conditional logic in assignemnts...

IOWs, don't fix the comments - fix the code to be self documenting
to remove the need for comments...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: fix up the comment in xfs_dir2_isleaf

[Stephen Zhang]

Dave Chinner <david@fromorbit.com> 于2022年9月12日周一 06:20写道：
>
> The "*vp" parameter should be a "bool *isleaf", in which case the
> return value is obvious and the comment can be removed. Then the
> logic in the function can be cleaned up to be obvious instead of
> relying on easy to mistake conditional logic in assignemnts...

Thanks for the suggestion.In order to make sure we are at the same page,
so this change will be shown like:
====
 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;

        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;
+       *isblock = XFS_FSB_TO_B(args->dp->i_mount, last) == args->geo->blksize;
        if (XFS_IS_CORRUPT(args->dp->i_mount,
-                          rval != 0 &&
+                          *isblock &&
                           args->dp->i_disk_size != args->geo->blksize))
                return -EFSCORRUPTED;
-       *vp = rval;
        return 0;
 }
====

Thanks,

Stephen.

Re: [PATCH] xfs: fix up the comment in xfs_dir2_isleaf

[Dave Chinner]

On Mon, Sep 12, 2022 at 11:14:51AM +0800, Stephen Zhang wrote:
> Dave Chinner <david@fromorbit.com> 于2022年9月12日周一 06:20写道：
> >
> > The "*vp" parameter should be a "bool *isleaf", in which case the
> > return value is obvious and the comment can be removed. Then the
> > logic in the function can be cleaned up to be obvious instead of
> > relying on easy to mistake conditional logic in assignemnts...
> 
> Thanks for the suggestion.In order to make sure we are at the same page,
> so this change will be shown like:

That's not what I was thinking. Cleanup involves converting
everything over to standard formatting and conventions. It also
means rethinking the logic to make the code more correct, easier to
read and understand, and so involves more than just changing the
name of a variable.

> ====
>  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;
> 
>         if ((rval = xfs_bmap_last_offset(args->dp, &last, XFS_DATA_FORK)))
>                 return rval;

We don't put assingments in if statements anymore, so this needs to
be rewritten in the form:

	error = foo();
	if (error) {
		....
		return error;
	}

> -       rval = XFS_FSB_TO_B(args->dp->i_mount, last) == args->geo->blksize;
> +       *isblock = XFS_FSB_TO_B(args->dp->i_mount, last) == args->geo->blksize;

Similarly, we don't elide if() statements in this way anymore,
because it's easy to mistake this code as a multiple assignment
rather than a combination of assignment and logic. if() is much
clearer.

>         if (XFS_IS_CORRUPT(args->dp->i_mount,
> -                          rval != 0 &&
> +                          *isblock &&
>                            args->dp->i_disk_size != args->geo->blksize))
>                 return -EFSCORRUPTED;

And this only ever evaluates as true if *isblock is true, so why
run this logic check when *isblock is false?

IOWs, we can rearrange the logic so that it's made up of simple,
individual single comparisons that are obviously self documenting:

int
xfs_dir2_isblock(
	struct xfs_da_args	*args,
	bool			*isblock)
{
	struct xfs_mount	*mp = args->dp->i_mount;
	xfs_fileoff_t		eof;
	int			error;

	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;
	return 0;
}


-- 
Dave Chinner
david@fromorbit.com