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

[Dave Chinner <david@fromorbit.com>]

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